clean up unit-test errors when it timeouts #697

lanchongyizu · 2017-07-17T08:33:58Z

Backgroud
This is for the story https://rackhd.atlassian.net/browse/RAC-5639.

During work of https://rackhd.atlassian.net/browse/RAC-5593, we found many unit test cases won't do the clean-up work if running into failure (aka exception handler) , so the plan is to add clean-up work in unit test cases.

DONE

add helper.httpServerBefore and helper.httpServerAfter as common
methods to start and stop mock HTTP server.
fix the issue that some unit test file doesn't stop mock HTTP server
when starting mock server timeouts.
add global sandbox in beforeEach, and restore it in afterEach
remove unnecessary resetStubs and restoreStubs

TEST
It's tested in sandbox:
http://10.62.59.175:8080/job/on-core_jamie/

@RackHD/corecommitters

houndci-bot · 2017-07-17T08:34:15Z

spec/lib/api/2.0/workflowGraphs-spec.js

+        workflowApiService.workflowsGetGraphsByName = this.sandbox.stub(),
+        workflowApiService.defineTaskGraph = this.sandbox.stub(),
+        workflowApiService.destroyGraphDefinition = this.sandbox.stub()
+        arpCache.getCurrent =  this.sandbox.stub().resolves([])


Missing semicolon.

houndci-bot · 2017-07-17T08:34:15Z

spec/lib/api/2.0/workflowGraphs-spec.js

+        workflowApiService.getGraphDefinitions = this.sandbox.stub(),
+        workflowApiService.workflowsGetGraphsByName = this.sandbox.stub(),
+        workflowApiService.defineTaskGraph = this.sandbox.stub(),
+        workflowApiService.destroyGraphDefinition = this.sandbox.stub()


Missing semicolon.

yyscamper · 2017-07-18T01:40:02Z

spec/helper.js

+        return helper.startServer(overrides, endpointOpt);
+    });
+
+    beforeEach(function () {


Since on-core helper.js has already create the sandbox, my suggestion is directly calling on-core's before (https://github.com/RackHD/on-core/blob/master/spec/helper.js#L268) rather than writing some duplicated code.

I think your idea is calling

helper.before(); helper.httpServerBefore();

instead of

helper.httpServerBefore();

But we don't need helper.start in on-http, so none of the unit test files calls helper.before.

yyscamper · 2017-07-18T01:40:54Z

spec/helper.js

+};
+
+helper.httpServerAfter = function() {
+    after('helper.httpServer.after', function() {


How about moving this to httpServerBefore, so that the unit-test doesn't need to call the httpServerAfter

It doesn't make sense here if we want to insert an after block before helper.httpServer.after.
E.g.:

after(function() { //call something }); helper.httpServerAfter();

yyscamper · 2017-07-18T01:42:45Z

spec/lib/api/2.0/config-spec.js

-    afterEach('tear down mocks', function () {
-        configuration.set.reset();
-        configuration.getAll.reset();
+    helper.httpServerBefore([]);


It would be better to remove the argument [], since an empty array seems redundant

anhou · 2017-07-31T06:16:12Z

spec/lib/api/2.0/ibms-spec.js

-        }
+    before(function () {
+        waterline = helper.injector.get('Services.Waterline');
+        Errors = helper.injector.get('Errors');helper.injector.get("Services.Configuration");


what's the purpose of 'helper.injector.get("Services.Configuration");'?

Good finding, it's not my purpose here. It's from the original code, and I think we should delete it.

anhou · 2017-07-31T06:17:43Z

@lanchongyizu Could you help post the errors in the RAC or in this PR's message? I see lots of minor changes which is not related to exception handler and this PR's topic

anhou · 2017-07-31T06:52:50Z

spec/helper.js


+helper.httpServerBefore = function(overrides, endpointOpt) {
+    before('helper.httpServer.before', function() {
+        this.timeout(5000);


I see that some cases are setting timeout to 10000ms previously, if all are set to 5000ms, is it sure that it doesn't have random issues?

Previously, they might include other tasks, so it was set to 10000ms, but here we just start HTTP server, so it is all set to 5000ms.

After group discussion, it will be set to 10000ms.

JenkinsRHD · 2017-07-31T07:30:10Z

BUILD on-http #359 : FAILURE

BUILD on-http #359 Error Logs ▼

Test Name: Redfish Systems Root should return a valid Redfish storage list Error Details: expected 200 "OK", got 500 "Internal Server Error" Response body: { error: { code: 'Base.1.0.GeneralError', message: 'A general error has occurred. See ExtendedInfo for more information.', '@Message.ExtendedInfo': [ [Object], [Object] ] } } Error: expected 200 "OK", got 500 "Internal Server Error" at Test._assertStatus (/home/jenkins/workspace/on-http/build-deps/on-http/node_modules/supertest/lib/test.js:232:12) at Test._assertFunction (/home/jenkins/workspace/on-http/build-deps/on-http/node_modules/supertest/lib/test.js:247:11) at Test.assert (/home/jenkins/workspace/on-http/build-deps/on-http/node_modules/supertest/lib/test.js:148:18) at assert (/home/jenkins/workspace/on-http/build-deps/on-http/node_modules/supertest/lib/test.js:127:12) at /home/jenkins/workspace/on-http/build-deps/on-http/node_modules/supertest/lib/test.js:124:5 at Test.Request.callback (/home/jenkins/workspace/on-http/build-deps/on-http/node_modules/supertest/node_modules/superagent/lib/node/index.js:703:3) at IncomingMessage.<anonymous> (/home/jenkins/workspace/on-http/build-deps/on-http/node_modules/supertest/node_modules/superagent/lib/node/index.js:922:12) at emitNone (events.js:72:20) at IncomingMessage.emit (events.js:166:7) at endReadableNT (_stream_readable.js:923:12) at nextTickCallbackWith2Args (node.js:458:9) at process._tickDomainCallback (node.js:413:17) Stack Trace: Error: expected 200 "OK", got 500 "Internal Server Error" at Test._assertStatus (node_modules/supertest/lib/test.js:232:12) at Test._assertFunction (node_modules/supertest/lib/test.js:247:11) at Test.assert (node_modules/supertest/lib/test.js:148:18) at assert (node_modules/supertest/lib/test.js:127:12) at node_modules/supertest/lib/test.js:124:5 at Test.Request.callback (node_modules/supertest/node_modules/superagent/lib/node/index.js:703:3) at IncomingMessage.<anonymous> (node_modules/supertest/node_modules/superagent/lib/node/index.js:922:12) at endReadableNT (_stream_readable.js:923:12)

lanchongyizu · 2017-07-31T07:32:03Z

The main errors are in SH sandbox Jenkins, such as http://10.62.59.175:8080/blue/organizations/jenkins/on-core_jamie/detail/on-core_jamie/29/tests

JenkinsRHD · 2017-08-01T16:16:23Z

BUILD on-http #368 : FAILURE

iceiilin · 2017-08-02T07:19:10Z

spec/lib/api/2.0/catalogs-spec.js

+        Errors = helper.injector.get('Errors');
+        waterline = helper.injector.get('Services.Waterline');
        return helper.injector.get('Views').load();
    });


I see two "before" here after this modification. Will they be called by parallel? If yes, as the previous code is a promise chain, will this change bring any race condition during the initialization?

No, they are called one by one.

iceiilin · 2017-08-02T07:24:38Z

spec/helper.js


 helper.stopServer = function () {
-    return helper.injector.get('app').stop();
+    if (helper.injector) {


I am curious about the case when helper.injector is null, since all the logic in https://github.com/RackHD/on-core/blob/master/spec/helper.js#L123 looks synchronized.

We could imagine that some unhandled exceptions are thrown before https://github.com/RackHD/on-core/blob/master/spec/helper.js#L163, then mocha will go on with the after section, then helper.injector would be undefined.

iceiilin · 2017-08-02T09:01:04Z

spec/lib/api/login/login-spec.js

        waterline = helper.injector.get('Services.Waterline');
        waterline.localusers = {
-            findOne: sinon.stub()
+            findOne: sandbox.stub()


Do we need to use "this.sandbox" here to align with other files?

For this specific file, it defines its own sandbox, so we keep it here. And before section doesn't have this.sandbox, which only exists in beforeEach section.

iceiilin · 2017-08-02T09:06:40Z

spec/lib/api/redfish-1.0/event-service-spec.js

-    before('start HTTP server', function () {
-        this.timeout(5000);
+
+    before(function() {


Can this before be combined with this one https://github.com/RackHD/on-http/pull/697/files#diff-cf0f2855402ebeda8abc6126044353caR54 ?

Originally, this is to stub redfishTool before httpServer is started, so we can't combine these two before section.

1) add helper.httpServerBefore and helper.httpServerAfter as common methods to start and stop mock HTTP server. 2) fix the issue that some unit test file doesn't stop mock HTTP server when starting mock server timeouts. 3) add global sandbox in beforeEach, and restore it in afterEach 4) remove unnecessary resetStubs and restoreStubs

houndci-bot reviewed Jul 17, 2017

View reviewed changes

lanchongyizu mentioned this pull request Jul 17, 2017

fix createIndexes problems RackHD/on-core#284

Closed

yyscamper reviewed Jul 18, 2017

View reviewed changes

lanchongyizu force-pushed the bugfix/unit-test branch 2 times, most recently from 2fb94e2 to 44e16d4 Compare July 24, 2017 05:26

lanchongyizu force-pushed the bugfix/unit-test branch from 44e16d4 to ff4e602 Compare July 27, 2017 08:53

lanchongyizu changed the title ~~stop mock HTTP server when starting it timeouts~~ clean up unit-test errors when it timeouts Jul 27, 2017

lanchongyizu force-pushed the bugfix/unit-test branch from ff4e602 to e7a6192 Compare July 28, 2017 06:26

anhou reviewed Jul 31, 2017

View reviewed changes

lanchongyizu force-pushed the bugfix/unit-test branch from e7a6192 to ef1f66c Compare July 31, 2017 07:21

lanchongyizu force-pushed the bugfix/unit-test branch 3 times, most recently from 711a66a to 33b6a09 Compare August 1, 2017 09:10

lanchongyizu force-pushed the bugfix/unit-test branch from 33b6a09 to f69b016 Compare August 2, 2017 08:36

iceiilin reviewed Aug 2, 2017

View reviewed changes

lanchongyizu force-pushed the bugfix/unit-test branch from f69b016 to 2e91df5 Compare August 2, 2017 10:01

iceiilin approved these changes Aug 3, 2017

View reviewed changes

pengz1 approved these changes Aug 3, 2017

View reviewed changes

iceiilin merged commit 1f9aa49 into RackHD:master Aug 3, 2017

clean up unit-test errors when it timeouts #697

clean up unit-test errors when it timeouts #697

Uh oh!

Conversation

lanchongyizu commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anhou commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anhou Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JenkinsRHD commented Jul 31, 2017

Uh oh!

lanchongyizu commented Jul 31, 2017

Uh oh!

JenkinsRHD commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanchongyizu Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lanchongyizu commented Jul 17, 2017 •

edited

Loading

anhou commented Jul 31, 2017 •

edited

Loading

anhou Jul 31, 2017 •

edited

Loading

lanchongyizu Aug 2, 2017 •

edited

Loading