launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07862
[Merge] lp:~jtv/maas/bug-963090 into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-963090 into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #963090 in MAAS: "testUpdateNodeUpdating has a timing-dependent assertion"
https://bugs.launchpad.net/maas/+bug/963090
For more details, see:
https://code.launchpad.net/~jtv/maas/bug-963090/+merge/105804
The node_views JavaScript tests used a problematic idiom for awaiting events:
1. Set up.
2. Trigger events.
3. this.wait(callback, timeout).
4. The callback contains the assertions.
The wait was sometimes too short, resulting in false test failures; and usually too long, resulting in slow tests. The proper way to do this is to register a callback that passes the assertions to this.resume():
1. Set up.
2. Register event handler that calls this.resume(callback).
3. The callback contains the assertions.
4. Trigger events.
5. this.wait().
Usually the event in question is the fade-in animation, so I could just hook into that. But the change revealed many other small problems. Among them that tests shared the same DOM fixtures, not all of which were cleaned up between tests. In this branch I make each instance of the dashboard use its own little DOM tree that it hooks into a placeholder in the HTML (thanks Raphaël for the x-template trick, and feedback on how to solve some of the problems). Each gets its own unique srcNode id to help avoid accidental sharing.
I made the dashboard widget do its node lookups relative to its srcNode, rather than in the full document. srcNode itself becomes a Node rather than a node selector. We get a bit more isolation this way, making for safer concurrency and letting us have multiple dashboards on the same test page simultaneously (as one test does) without risk of confusion. It also reduces reliance on the dashboard being included in the document. I would have liked to have taken that further but for some reason I can't as yet fathom, the hover tests really need to get their nodes from Y.one().
I also did more of the node lookups in advance, in the initial part of each test, so that all the stuff that might be affected by concurrent tests happens in the first, synchronous part of the test. I don't know how far the YUI test framework takes concurrency, and this is a cheap safety belt.
Some of the tests fired Node.deleted and Node.created events, as we do through txlongpoll. These are global events though, and were spilling over into other tests (which makes for very confusing errors I can tell you!) so I moved the test down one call level. The event handlers for these global events only called a single method, so the tests now call that single method. Test coverage suffers a bit, but a proper test for this would cover integration between txlongpoll and the dashboard, not just the one thin call I'm skipping.
One bit of testing didn't make sense at all: “// Test the animation doesn't run if we have set it to run.” The comment alone confuses me. But the test was not one that could ever fail (it initialized some variables to false, did some unrelated work, and then checked that the variables were still false) and I don't see a really proper way to test that animations don't run. In the end, I just didn't bother and removed that bit of testing. Normally I'm all for negative tests, but with YUI often the best we can do is some integration tests.
You may also notice that I replaced some /* multi-line comments */ with // end-of-line comments.
That's not so much a choice of style as one of convenience: I did a lot of commenting-out for this branch and that will break with multi-line comments, since the construct does not nest.
Jeroen
--
https://code.launchpad.net/~jtv/maas/bug-963090/+merge/105804
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-963090 into lp:maas.
=== modified file 'src/maasserver/static/js/node_views.js'
--- src/maasserver/static/js/node_views.js 2012-05-11 11:05:45 +0000
+++ src/maasserver/static/js/node_views.js 2012-05-15 11:55:23 +0000
@@ -143,17 +143,17 @@
retired_template: ('{nodes} retired node{plural} not represented.'),
initializer: function(config) {
- this.srcNode = config.srcNode;
- this.summaryNode = Y.one(config.summaryNode);
- this.numberNode = Y.one(config.numberNode);
- this.descriptionNode = Y.one(config.descriptionNode);
- this.reservedNode = Y.one(config.reservedNode);
- /* XXX: GavinPanella 2012-04-17 bug=984117:
- * Hidden until we support reserved nodes. */
+ this.srcNode = Y.one(config.srcNode);
+ this.summaryNode = this.srcNode.one(config.summaryNode);
+ this.numberNode = this.srcNode.one(config.numberNode);
+ this.descriptionNode = this.srcNode.one(config.descriptionNode);
+ this.reservedNode = this.srcNode.one(config.reservedNode);
+ // XXX: GavinPanella 2012-04-17 bug=984117:
+ // Hidden until we support reserved nodes.
this.reservedNode.hide();
- this.retiredNode = Y.one(config.retiredNode);
- /* XXX: GavinPanella 2012-04-17 bug=984116:
- * Hidden until we support retired nodes. */
+ this.retiredNode = this.srcNode.one(config.retiredNode);
+ // XXX: GavinPanella 2012-04-17 bug=984116:
+ // Hidden until we support retired nodes.
this.retiredNode.hide();
this.deployed_nodes = 0;
this.commissioned_nodes = 0;
@@ -255,7 +255,7 @@
},
loadNodesStarted: function() {
- Y.one(this.srcNode).insert(this.spinnerNode, 0);
+ this.srcNode.insert(this.spinnerNode, 0);
},
loadNodesEnded: function() {
@@ -378,12 +378,12 @@
var text = Y.Lang.sub(template, {plural: plural});
if (animate) {
- this.fade_out.run();
this.fade_out.on('end', function (e, self, nodes, text) {
self.numberNode.setContent(nodes);
self.descriptionNode.setContent(text);
self.fade_in.run();
}, null, this, nodes, text);
+ this.fade_out.run();
}
else {
this.numberNode.setContent(nodes);
=== modified file 'src/maasserver/static/js/nodes_chart.js'
--- src/maasserver/static/js/nodes_chart.js 2012-04-17 15:48:27 +0000
+++ src/maasserver/static/js/nodes_chart.js 2012-05-15 11:55:23 +0000
@@ -31,10 +31,10 @@
NodesChartWidget.ATTRS = {
/**
- * The node id to display the chart.
+ * The node or node id to display the chart.
*
* @attribute node_id
- * @type string
+ * @type Node or string
*/
node_id: {
value: ''
=== modified file 'src/maasserver/static/js/tests/test_node_views.html'
--- src/maasserver/static/js/tests/test_node_views.html 2012-04-30 13:49:53 +0000
+++ src/maasserver/static/js/tests/test_node_views.html 2012-05-15 11:55:23 +0000
@@ -30,7 +30,9 @@
</head>
<body>
<span id="suite">maas.node_views.tests</span>
- <div id="dashboard">
+
+ <!-- Reusable template: nodes for the dashboard to hook into. -->
+ <script type="text/x-template" id="dashboard-hooks">
<div id="chart"></div>
<div id="summary">
<h2 id="nodes-number"></h2>
@@ -38,6 +40,10 @@
</div>
<p id="reserved-nodes"></p>
<p id="retired-nodes"></p>
- </div>
+ </script>
+
+ <!-- Place for tests to hook instances of the template in the DOM. -->
+ <div id="placeholder"></div>
+
</body>
</html>
=== modified file 'src/maasserver/static/js/tests/test_node_views.js'
--- src/maasserver/static/js/tests/test_node_views.js 2012-05-14 08:27:36 +0000
+++ src/maasserver/static/js/tests/test_node_views.js 2012-05-15 11:55:23 +0000
@@ -10,6 +10,11 @@
var module = Y.maas.node_views;
var suite = new Y.Test.Suite("maas.node_views Tests");
+
+// Dump this HTML into #placeholder to get DOM hooks for the dashboard.
+var dashboard_hooks = Y.one('#dashboard-hooks').getContent()
+
+
suite.add(new Y.maas.testing.TestCase({
name: 'test-node-views-NodeListLoader',
@@ -55,6 +60,7 @@
name: 'test-node-views-NodeDashBoard',
setUp : function () {
+ Y.one('#placeholder').empty();
var NODE_STATUS = Y.maas.enums.NODE_STATUS;
this.data = [
{
@@ -126,6 +132,18 @@
},
/**
+ * Counter to generate unique numbers.
+ */
+ counter: 0,
+
+ /**
+ * Get next value of this.counter, and increment.
+ */
+ getNumber: function() {
+ return this.counter++;
+ },
+
+ /**
* Create a dashboard view, render it, and arrange for its cleanup.
*
* The "data" parameter defaults to this.data.
@@ -134,7 +152,12 @@
if (data === undefined) {
data = this.data;
}
- var view = create_dashboard_view(data, this);
+ var root_node_id = 'widget-' + this.getNumber().toString();
+ var new_dash = Y.Node.create('<div />').set('id', root_node_id);
+ this.addCleanup(function() { new_dash.remove(); });
+ new_dash.append(Y.Node.create(dashboard_hooks));
+ Y.one('#placeholder').append(new_dash);
+ var view = create_dashboard_view(data, this, '#' + root_node_id);
this.addCleanup(function() { view.destroy(); });
view.render();
return view;
@@ -144,41 +167,56 @@
var view = this.makeDashboard();
Y.Assert.areNotEqual(
'',
- Y.one('#chart').get('text'),
+ view.srcNode.one('#chart').get('text'),
"The chart node should have been populated.");
},
testHoverMouseover: function() {
+ var test = this;
var view = this.makeDashboard();
-
- // Chart hovers should be set up
- Y.one(view.chart._offline_circle[0].node).simulate('mouseover');
- this.wait(function() {
- Y.Assert.areEqual(
- '4',
- Y.one('#nodes-number').get('text'),
- "The total number of offline nodes should be set.");
- Y.Assert.areEqual(
- 'nodes offline',
- Y.one('#nodes-description').get('text'),
- "The text should be set with nodes as a plural.");
- }, 500);
+ var number_node = view.srcNode.one('#nodes-number');
+ var description_node = view.srcNode.one('#nodes-description');
+
+ // The dashboard sets up hover actions on the chart. Hovering over a
+ // chart segment causes its stats to fade in.
+ view.fade_in.on('end', function() {
+ test.resume(function() {
+ Y.Assert.areEqual(
+ '4',
+ number_node.get('text'),
+ "The total number of offline nodes should be set.");
+ Y.Assert.areEqual(
+ 'nodes offline',
+ description_node.get('text'),
+ "The text should be set with nodes as a plural.");
+ });
+ });
+
+ Y.one(view.chart._offline_circle[0].node).simulate(
+ 'mouseover');
+ this.wait();
},
testHoverMouseout: function() {
+ var test = this;
var view = this.makeDashboard();
+ var number_node = view.srcNode.one('#nodes-number');
+ var description_node = view.srcNode.one('#nodes-description');
+ view.fade_in.on('end', function() {
+ test.resume(function() {
+ Y.Assert.areEqual(
+ '12',
+ number_node.get('text'),
+ "The total number of nodes should be set.");
+ Y.Assert.areEqual(
+ 'nodes in this MAAS',
+ description_node.get('text'),
+ "The default text should be set.");
+ });
+ });
Y.one(view.chart._offline_circle[0].node).simulate('mouseout');
- this.wait(function() {
- Y.Assert.areEqual(
- '12',
- Y.one('#nodes-number').get('text'),
- "The total number of nodes should be set.");
- Y.Assert.areEqual(
- 'nodes in this MAAS',
- Y.one('#nodes-description').get('text'),
- "The default text should be set.");
- }, 500);
+ this.wait();
},
testDisplay: function() {
@@ -210,75 +248,84 @@
"The number of retired nodes should have been set.");
Y.Assert.areEqual(
'12',
- Y.one('#nodes-number').get('text'),
+ view.srcNode.one('#nodes-number').get('text'),
"The total number of nodes should be set.");
Y.Assert.areEqual(
"nodes in this MAAS",
- Y.one('#nodes-description').get('text'),
+ view.srcNode.one('#nodes-description').get('text'),
"The summary text should be set.");
Y.Assert.areEqual(
"3 nodes reserved for named deployment.",
- Y.one('#reserved-nodes').get('text'),
+ view.srcNode.one('#reserved-nodes').get('text'),
"The reserved text should be set.");
- /* XXX: GavinPanella 2012-04-17 bug=984117:
- * Hidden until we support reserved nodes. */
+ // XXX: GavinPanella 2012-04-17 bug=984117:
+ // Hidden until we support reserved nodes.
Y.Assert.areEqual("none", view.reservedNode.getStyle("display"));
Y.Assert.areEqual(
"1 retired node not represented.",
- Y.one('#retired-nodes').get('text'),
+ view.srcNode.one('#retired-nodes').get('text'),
"The retired text should be set.");
- /* XXX: GavinPanella 2012-04-17 bug=984116:
- * Hidden until we support retired nodes. */
+ // XXX: GavinPanella 2012-04-17 bug=984116:
+ // Hidden until we support retired nodes.
Y.Assert.areEqual("none", view.retiredNode.getStyle("display"));
},
testUpdateNodeCreation: function() {
+ var test = this;
var view = this.makeDashboard();
var node = {
system_id: 'sys14',
hostname: 'host14',
status: Y.maas.enums.NODE_STATUS.DECLARED
};
+ var number_node = view.srcNode.one('#nodes-number');
Y.Assert.areEqual(
'12',
- Y.one('#nodes-number').get('text'),
+ number_node.get('text'),
"The total number of nodes should be set.");
- // Check node creation
+
+ // Check node creation.
Y.Assert.areEqual(
2,
view.added_nodes,
"Check the initial number of nodes for the status.");
- Y.fire('Node.created', {instance: node});
- Y.Assert.areEqual(
- 'host14',
- view.modelList.getById('sys14').get('hostname'),
- "The node should exist in the modellist.");
- Y.Assert.areEqual(
- 3,
- view.added_nodes,
- "The status should have one extra node.");
- Y.Assert.areEqual(
- 3,
- view.chart.get('added_nodes'),
- "The chart status number should also be updated.");
- var self = this;
- this.wait(function() {
- Y.Assert.areEqual(
- '13',
- Y.one('#nodes-number').get('text'),
- "The total number of nodes should have been updated.");
- }, 500);
+
+ view.fade_in.on('end', function() { test.resume(function() {
+ Y.Assert.areEqual(
+ 'host14',
+ view.modelList.getById('sys14').get('hostname'),
+ "The node should exist in the modellist.");
+ Y.Assert.areEqual(
+ 3,
+ view.added_nodes,
+ "The status should have one extra node.");
+ Y.Assert.areEqual(
+ 3,
+ view.chart.get('added_nodes'),
+ "The chart status number should also be updated.");
+ Y.Assert.areEqual(
+ '13',
+ number_node.get('text'),
+ "The total number of nodes should have been updated.");
+ });
+ });
+
+ view.updateNode('created', node);
+ this.wait();
},
testUpdateNodeUpdating: function() {
+ var test = this;
var view = this.makeDashboard();
var node = this.data[0];
+ var number_node = view.srcNode.one('#nodes-number');
node.status = Y.maas.enums.NODE_STATUS.ALLOCATED;
Y.Assert.areEqual(
1,
view.deployed_nodes,
"Check the initial number of nodes for the new status.");
- Y.fire('Node.updated', {instance: node});
+
+ view.updateNode('updated', node);
Y.Assert.areEqual(
6,
view.modelList.getById('sys1').get('status'),
@@ -299,43 +346,46 @@
1,
view.chart.get('added_nodes'),
"The old chart status number should also be updated.");
- /* XXX: Bug: 963090 This is timing dependant and causes spurious
- failures from time to time.
- this.wait(function() {
- Y.Assert.areEqual(
- Y.one('#nodes-number').get('text'),
- '12',
- "The total number of nodes should not have been updated.");
- }, 500);
- */
+ Y.Assert.areEqual(
+ number_node.get('text'),
+ '12',
+ "The total number of nodes should not have been updated.");
},
testUpdateNodeDeleting: function() {
+ var test = this;
var view = this.makeDashboard();
var node = this.data[12];
- Y.fire('Node.deleted', {instance: node});
- Y.Assert.isNull(
- view.modelList.getById('sys14'),
- "The node should have been deleted.");
- Y.Assert.areEqual(
- 1,
- view.deployed_nodes,
- "The status should have one less node.");
- Y.Assert.areEqual(
- 1,
- view.chart.get('deployed_nodes'),
- "The chart status number should also be updated.");
- this.wait(function() {
- Y.Assert.areEqual(
- '12',
- Y.one('#nodes-number').get('text'),
- "The total number of nodes should have been updated.");
- }, 500);
+ var number_node = view.srcNode.one('#nodes-number');
+
+ view.fade_in.on('end', function() {
+ test.resume(function() {
+ Y.Assert.isNull(
+ view.modelList.getById('sys14'),
+ "The node should have been deleted.");
+ Y.Assert.areEqual(
+ 1,
+ view.deployed_nodes,
+ "The status should have one less node.");
+ Y.Assert.areEqual(
+ 1,
+ view.chart.get('deployed_nodes'),
+ "The chart status number should also be updated.");
+ Y.Assert.areEqual(
+ '12',
+ number_node.get('text'),
+ "The total number of nodes should have been updated.");
+ });
+ });
+
+ view.updateNode('deleted', node);
+ this.wait();
},
testUpdateStatus: function() {
var view = this.makeDashboard();
+ var reserved_node = view.srcNode.one('#reserved-nodes');
// Add a node to a status that also updates the chart
Y.Assert.areEqual(
2,
@@ -375,7 +425,7 @@
"The status should have one extra node.");
Y.Assert.areEqual(
"4 nodes reserved for named deployment.",
- Y.one('#reserved-nodes').get('text'),
+ reserved_node.get('text'),
"The dashboard reserved text should be updated.");
Y.Assert.isFalse(
result,
@@ -393,11 +443,11 @@
view.setSummary(false);
Y.Assert.areEqual(
'1',
- Y.one('#nodes-number').get('text'),
+ view.srcNode.one('#nodes-number').get('text'),
"The total number of nodes should be set.");
Y.Assert.areEqual(
'node in this MAAS',
- Y.one('#nodes-description').get('text'),
+ view.srcNode.one('#nodes-description').get('text'),
"The text should be set with nodes as singular.");
// Test the default summary, with one node
@@ -405,11 +455,11 @@
view.setSummary(false);
Y.Assert.areEqual(
'12',
- Y.one('#nodes-number').get('text'),
+ view.srcNode.one('#nodes-number').get('text'),
"The total number of nodes should be set.");
Y.Assert.areEqual(
'nodes in this MAAS',
- Y.one('#nodes-description').get('text'),
+ view.srcNode.one('#nodes-description').get('text'),
"The text should be set with nodes as a plural.");
// Test we can set the summary for a particular status (multiple nodes)
@@ -417,28 +467,16 @@
view.setSummary(false, 1, view.queued_template);
Y.Assert.areEqual(
'1',
- Y.one('#nodes-number').get('text'),
+ view.srcNode.one('#nodes-number').get('text'),
"The total number of nodes should be set.");
Y.Assert.areEqual(
'node queued',
- Y.one('#nodes-description').get('text'),
+ view.srcNode.one('#nodes-description').get('text'),
"The text should be set with nodes as a plural.");
-
- // Test the animation doesn't run if we have set it to run
- var fade_out_anim = false;
- var fade_in_anim = false;
- view.setSummary(false);
- this.wait(function() {
- Y.Assert.isFalse(
- fade_out_anim,
- "The fade out animation should not have run.");
- Y.Assert.isFalse(
- fade_in_anim,
- "The fade in animation should not have run.");
- }, 500);
},
testSetSummaryAnimation: function() {
+ var test = this;
var view = this.makeDashboard();
var fade_out_anim = false;
var fade_in_anim = false;
@@ -447,16 +485,17 @@
});
view.fade_in.on('end', function() {
fade_in_anim = true;
+ test.resume(function() {
+ Y.Assert.isTrue(
+ fade_out_anim,
+ "The fade-out animation should have run.");
+ Y.Assert.isTrue(
+ fade_in_anim,
+ "The fade-in animation should have run.");
+ });
});
view.setSummary(true);
- this.wait(function() {
- Y.Assert.isTrue(
- fade_out_anim,
- "The fade-out animation should have run.");
- Y.Assert.isTrue(
- fade_in_anim,
- "The fade-in animation should have run.");
- }, 500);
+ this.wait();
},
testSetNodeText: function() {
@@ -465,7 +504,7 @@
view.reservedNode, view.reserved_template, view.reserved_nodes);
Y.Assert.areEqual(
"3 nodes reserved for named deployment.",
- Y.one('#reserved-nodes').get('text'),
+ view.srcNode.one('#reserved-nodes').get('text'),
"The text should be set with nodes as a plural.");
var data = [{
@@ -478,7 +517,7 @@
view.reservedNode, view.reserved_template, view.reserved_nodes);
Y.Assert.areEqual(
"1 node reserved for named deployment.",
- Y.one('#reserved-nodes').get('text'),
+ view.srcNode.one('#reserved-nodes').get('text'),
"The text should be set with nodes as singular.");
},
@@ -488,18 +527,16 @@
12,
view.getNodeCount(),
"The total nodes should not return retired nodes.");
- },
-
- tearDown : function () {
- Y.one('#chart').set('text', '');
}
+
}));
-function create_dashboard_view(data, self) {
+
+function create_dashboard_view(data, self, root_node_descriptor) {
var response = Y.JSON.stringify(data);
self.mockSuccess(response, module);
var view = new Y.maas.node_views.NodesDashboard({
- srcNode: '#dashboard',
+ srcNode: root_node_descriptor,
summaryNode: '#summary',
numberNode: '#nodes-number',
descriptionNode: '#nodes-description',
