Skip to content

Commit

Permalink
Combine state into compositeStatus
Browse files Browse the repository at this point in the history
These fields only differed by the inclusion of draining,
and the existing compositeStatus was only being used in
one place.

This removes an assertion that was unreliable with
certain random circumstances; it checked that the
composite status was equal to the node status, but that
wasn’t true for draining nodes. It felt okay to remove
because testing a generalised replacement would
necessitate reproducing the entire computed property and
there’s already an assertion that tests the specific
“ineligible” composite status.
  • Loading branch information
backspace committed Dec 11, 2019
1 parent ba6a758 commit ad3863f
Show file tree
Hide file tree
Showing 9 changed files with 28 additions and 32 deletions.
8 changes: 4 additions & 4 deletions ui/app/components/client-node-row.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ export default Component.extend(WithVisibilityDetection, {

watch: watchRelationship('allocations'),

stateClass: computed('node.state', function() {
let state = this.get('node.state');
compositeStatusClass: computed('node.compositeStatus', function() {
let compositeStatus = this.get('node.compositeStatus');

if (state === 'draining') {
if (compositeStatus === 'draining') {
return 'status-text is-info';
} else if (state === 'ineligible') {
} else if (compositeStatus === 'ineligible') {
return 'status-text is-warning';
} else {
return '';
Expand Down
2 changes: 1 addition & 1 deletion ui/app/controllers/clients/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import Searchable from 'nomad-ui/mixins/searchable';
import { serialize, deserializedQueryParam as selection } from 'nomad-ui/utils/qp-serialize';

export default Controller.extend(
SortableFactory(['id', 'name', 'state', 'datacenter']),
SortableFactory(['id', 'name', 'compositeStatus', 'datacenter']),
Searchable,
{
clientsController: controller('clients'),
Expand Down
6 changes: 1 addition & 5 deletions ui/app/models/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,7 @@ export default Model.extend({

// A status attribute that includes states not included in node status.
// Useful for coloring and sorting nodes
compositeStatus: computed('status', 'isEligible', function() {
return this.isEligible ? this.status : 'ineligible';
}),

state: computed('isDraining', 'isEligible', 'status', function() {
compositeStatus: computed('isDraining', 'isEligible', 'status', function() {
if (this.isDraining) {
return 'draining';
} else if (!this.isEligible) {
Expand Down
3 changes: 2 additions & 1 deletion ui/app/styles/components/node-status-light.scss
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ $size: 0.75em;
);
}

&.ineligible {
&.ineligible,
&.draining {
background: $warning;
}
}
2 changes: 1 addition & 1 deletion ui/app/templates/clients/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
<th class="is-narrow"></th>
{{#t.sort-by prop="id"}}ID{{/t.sort-by}}
{{#t.sort-by class="is-200px is-truncatable" prop="name"}}Name{{/t.sort-by}}
{{#t.sort-by prop="state"}}State{{/t.sort-by}}
{{#t.sort-by prop="compositeStatus"}}State{{/t.sort-by}}
<th>Address</th>
{{#t.sort-by prop="datacenter"}}Datacenter{{/t.sort-by}}
<th># Allocs</th>
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/components/client-node-row.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
</td>
<td data-test-client-id>{{#link-to "clients.client" node.id class="is-primary"}}{{node.shortId}}{{/link-to}}</td>
<td data-test-client-name class="is-200px is-truncatable" title="{{node.name}}">{{node.name}}</td>
<td data-test-client-state>
<td data-test-client-composite-status>
<span class="tooltip" aria-label="{{node.status}} / {{if node.isDraining "draining" "not draining"}} / {{if node.isEligible "eligible" "not eligible"}}">
<span class="{{stateClass}}">{{node.state}}</span>
<span class="{{compositeStatusClass}}">{{node.compositeStatus}}</span>
</span>
</td>
<td data-test-client-address>{{node.httpAddr}}</td>
Expand Down
5 changes: 0 additions & 5 deletions ui/tests/acceptance/client-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,6 @@ module('Acceptance | client detail', function(hooks) {

assert.ok(ClientDetail.title.includes(node.name), 'Title includes name');
assert.ok(ClientDetail.title.includes(node.id), 'Title includes id');
assert.equal(
ClientDetail.statusLight.objectAt(0).id,
node.status,
'Title includes status light'
);
});

test('/clients/:id should list additional detail for the node below the title', async function(assert) {
Expand Down
26 changes: 15 additions & 11 deletions ui/tests/acceptance/clients-list-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ module('Acceptance | clients list', function(hooks) {

assert.equal(nodeRow.id, node.id.split('-')[0], 'ID');
assert.equal(nodeRow.name, node.name, 'Name');
assert.equal(nodeRow.state.text, 'draining', 'Combined status, draining, and eligbility');
assert.equal(
nodeRow.compositeStatus.text,
'draining',
'Combined status, draining, and eligbility'
);
assert.equal(nodeRow.address, node.httpAddr);
assert.equal(nodeRow.datacenter, node.datacenter, 'Datacenter');
assert.equal(nodeRow.allocations, allocations.length, '# Allocations');
Expand Down Expand Up @@ -81,24 +85,24 @@ module('Acceptance | clients list', function(hooks) {

await ClientsList.visit();

ClientsList.nodes[0].state.as(readyClient => {
ClientsList.nodes[0].compositeStatus.as(readyClient => {
assert.equal(readyClient.text, 'ready');
assert.ok(readyClient.isUnformatted, 'expected no status class');
assert.equal(readyClient.tooltip, 'ready / not draining / eligible');
});

assert.equal(ClientsList.nodes[1].state.text, 'initializing');
assert.equal(ClientsList.nodes[2].state.text, 'down');
assert.equal(ClientsList.nodes[1].compositeStatus.text, 'initializing');
assert.equal(ClientsList.nodes[2].compositeStatus.text, 'down');

assert.equal(ClientsList.nodes[3].state.text, 'ineligible');
assert.ok(ClientsList.nodes[3].state.isWarning, 'expected warning class');
assert.equal(ClientsList.nodes[3].compositeStatus.text, 'ineligible');
assert.ok(ClientsList.nodes[3].compositeStatus.isWarning, 'expected warning class');

assert.equal(ClientsList.nodes[4].state.text, 'draining');
assert.ok(ClientsList.nodes[4].state.isInfo, 'expected info class');
assert.equal(ClientsList.nodes[4].compositeStatus.text, 'draining');
assert.ok(ClientsList.nodes[4].compositeStatus.isInfo, 'expected info class');

await ClientsList.sortBy('state');
await ClientsList.sortBy('compositeStatus');

assert.deepEqual(ClientsList.nodes.mapBy('state.text'), [
assert.deepEqual(ClientsList.nodes.mapBy('compositeStatus.text'), [
'ready',
'initializing',
'ineligible',
Expand All @@ -115,7 +119,7 @@ module('Acceptance | clients list', function(hooks) {

await settled();

assert.deepEqual(ClientsList.nodes.mapBy('state.text'), [
assert.deepEqual(ClientsList.nodes.mapBy('compositeStatus.text'), [
'initializing',
'ineligible',
'ineligible',
Expand Down
4 changes: 2 additions & 2 deletions ui/tests/pages/clients/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ export default create({
id: text('[data-test-client-id]'),
name: text('[data-test-client-name]'),

state: {
scope: '[data-test-client-state]',
compositeStatus: {
scope: '[data-test-client-composite-status]',

tooltip: attribute('aria-label', '.tooltip'),

Expand Down

0 comments on commit ad3863f

Please sign in to comment.