pollers unit-test for 2.0 and services #134

srinia6 · 2016-02-17T19:49:20Z

@dalebremner @brianparry @geoff-reid @abildw @RackHD/corecommitters

dalebremner · 2016-02-17T20:48:39Z

👍

jlongever · 2016-02-17T21:04:47Z

Interesting, no coveralls coverage check?

srinia6 · 2016-02-17T21:32:06Z

@jlongever Does it need to be turned on ? If so, how?
FYI - this is addition to this PR - #133

jlongever · 2016-02-17T21:38:20Z

@srinia6 it should be on by default. not sure why it didn't run.

BillyAbildgaard · 2016-02-18T14:06:03Z

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

@@ -0,0 +1,330 @@
+// Copyright 2015, EMC, Inc.


update to 2016

BillyAbildgaard · 2016-02-18T14:10:36Z

I just had a minor comment about the date, but other than that it looks good to me

👍

yyscamper · 2016-02-19T17:10:39Z

spec/lib/services/pollers-api-service-spec.js

+            destroyByIdentifier: sinon.stub().resolves()
+        };
+
+	    taskProtocol = helper.injector.get("Protocol.Task");


indent error

yyscamper · 2016-02-19T17:47:14Z

👎 for this, since I see a lot bad test case design. @srinia6 please help fix this

houndci-bot · 2016-02-22T20:26:34Z

spec/lib/services/pollers-api-service-spec.js

+
+    describe("patchPollersById", function() {
+        it('should expose the appropriate methods', function() {
+            pollerService.should.have.property('patchPollersById')


Mixed double and single quotes.
Too many errors. (53% scanned).

brianparry · 2016-02-22T22:39:42Z

spec/lib/services/pollers-api-service-spec.js

+         });
+
+        it("should return error if specific poller info is not found", function () {
+            var mockPoller = {"message":"NotFoundError: Could not find workitem with identifier"};


@srinia6 I think we want to make this an error object: var mockPollerError = new Errors.NotFoundError('Could not find workitem with identifier').

houndci-bot · 2016-02-23T13:52:07Z

spec/lib/services/pollers-api-service-spec.js

+        it("should return error if specific poller info is not found", function () {
+            var mockPollerError = new Errors.NotFoundError("Could not find workitem with identifier");
+            waterline.workitems.needByIdentifier.rejects(mockPollerError);
+            return pollerService.getPollersById().should.eventually.be.rejectedWith(mockPollerError);


Line is too long.

Ignoring this error.

jlongever · 2016-02-23T15:03:57Z

LGTM +1

yyscamper · 2016-02-24T01:38:49Z

👍 after fix the hounci warning "Line is too long"

brianparry · 2016-02-24T01:54:58Z

👍

anhou · 2016-02-24T01:55:58Z

@srinia6 all houndci's review comments should be fixed. if it could be ignored, please comment it.

👍 after fix all houndci issues

yyscamper · 2016-02-24T02:04:19Z

spec/lib/services/pollers-api-service-spec.js

+            }];
+            waterline.workitems.needByIdentifier.withArgs({"id": "4532"}).resolves(mockPoller[0]);
+            waterline.workitems.needByIdentifier.withArgs({"id": "1234"}).resolves(mockPoller[1]);
+            Promise.all(["4532", "1234"].map(function(id, index) {


just realized this promise has not been returned, this should be fixed before merge.

yyscamper · 2016-02-24T02:43:10Z

I have to withdraw my approval, because I still see a critical error:
(1) The Promise.all need be returned.
(2) The houndci warning about index and semicolon need be fixed, because it reflects a critical error that the brackets don't match. (I wonder why the travis ci pass the test).

Meanwhile, the hounci warning about "Line is too long" should also be fixed.

houndci-bot · 2016-02-24T21:05:51Z

spec/lib/services/pollers-api-service-spec.js

+         });
+
+        it("should return error if specific poller info is not found", function () {
+            var mockPollerError = new Errors.NotFoundError("Could not find workitem with identifier");


Line is too long.

Ignoring this error.

JenkinsRHD · 2016-02-24T21:32:46Z

*** BUILD #467 ***

srinia6 · 2016-02-24T23:29:21Z

test this please

yyscamper · 2016-02-25T00:24:27Z

@srinia6, Ithink you miss another fix: the promise.map need be returned otherwise your assertion logic will not take effect.

Meanwhile, for "Line is too long", if you do have reason to ignore it, you'd use jshint ignore synatx(please search jshint manual) and append it after the line you want to ingore.

srinia6 · 2016-02-25T00:41:39Z

@yyscamper I looked at the test case. It is working as expected. The reason it was passing previously was the data [resolved] came back as undefined, and the index also was un-defined [so, the test case passed] .
This was fixed by replacing ["id" : "1234" ] with ["1234"]. Also, the brackets were closed appropriately. [from the beginning ]@brianparry the build passed.

yyscamper · 2016-02-25T05:12:34Z

@srinia6 : understand the brackets design, your current code is OK for me.
My primary concern is that the promise in the test case doesn't be returned. You can do an experiment if not return the Promise. Just change the "expect(xxx).to.equal(xxx)" to the inverted logic "expect(xxx).to.not.equal(xxx)", you will find both pass!!

This is not a style problem, this is a problem about library/framework usage, so this need be fixed. If you add your assertion logic in the Promise, that promise must be returned, otherwise mocha will not wait the test case is actually done. I see all your code's Promises are returned except this one. Please refer to the mocha guideline about Promise: https://mochajs.org/#working-with-promises.

yyscamper · 2016-02-25T05:13:39Z

👍 after fix the problem that Promise doesn't been returned

srinia6 · 2016-02-25T14:01:31Z

@yyscamper : Not sure if I understood your concern. You suggested these changes!

I did try substituting per your suggestion/concern i.e.- "** Just change the "expect(xxx).to.equal(xxx)" to the inverted logic "expect(xxx).to.not.equal(xxx)", you will find both pass!! **"

I see "crisp" error showing-up as below:

Unhandled rejection AssertionError: expected { Object (id, name, ...) } to not equal { Object (id, name, ...) }
at .........
at arrayEach (.....)
at Function. (.....)
at .........
at tryCatcher (.......)
at Promise._settlePromiseFromHandler (.....)
at Promise._settlePromiseAt (.....)
at Promise._settlePromises (....)
at Async._drainQueue (......)
at Async._drainQueues (....)
at Async.drainQueues (,,,,)
at process._tickDomainCallback (node.js:459:13)

SO i am guessing it IS doing all the "right" thing?

srinia6 · 2016-02-25T15:35:56Z

@yyscamper Fixed it. Turns out that i am not seeing the failing case because the promise is mocked-out (here).

yyscamper · 2016-02-25T16:00:11Z

good 👍 . @srinia6

srinia6 · 2016-02-25T18:44:37Z

@RackHD/corecommitters - Can you review/merge this please?

jlongever · 2016-02-25T20:08:34Z

+1

pollers unit-test for 2.0 and services

pollers unit-test

876cde8

BillyAbildgaard reviewed Feb 18, 2016
View reviewed changes

spec/lib/api/2.0/pollers-spec.js Outdated

@@ -0,0 +1,330 @@

// Copyright 2015, EMC, Inc.

Copy link

BillyAbildgaard Feb 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update to 2016

fix year

aedeeab

yyscamper reviewed Feb 19, 2016
View reviewed changes

Address review comments

fe9e5f1

houndci-bot reviewed Feb 22, 2016
View reviewed changes

brianparry reviewed Feb 22, 2016
View reviewed changes

address new review comments

774e915

houndci-bot reviewed Feb 23, 2016
View reviewed changes

yyscamper reviewed Feb 24, 2016
View reviewed changes

Fix code

b0e4a10

houndci-bot reviewed Feb 24, 2016
View reviewed changes

Address review comment

6fcbeeb

jlongever added a commit that referenced this pull request Feb 25, 2016

Merge pull request #134 from srinia6/feature/swagger-pollers-route

b0e2550

pollers unit-test for 2.0 and services

jlongever merged commit b0e2550 into RackHD:master Feb 25, 2016

srinia6 deleted the feature/swagger-pollers-route branch March 28, 2016 15:27

pollers unit-test for 2.0 and services #134

pollers unit-test for 2.0 and services #134

Uh oh!

Conversation

srinia6 commented Feb 17, 2016

Uh oh!

dalebremner commented Feb 17, 2016

Uh oh!

jlongever commented Feb 17, 2016

Uh oh!

srinia6 commented Feb 17, 2016

Uh oh!

jlongever commented Feb 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillyAbildgaard commented Feb 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyscamper commented Feb 19, 2016

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!

jlongever commented Feb 23, 2016

Uh oh!

yyscamper commented Feb 24, 2016

Uh oh!

brianparry commented Feb 24, 2016

Uh oh!

anhou commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyscamper commented Feb 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JenkinsRHD commented Feb 24, 2016

Uh oh!

srinia6 commented Feb 24, 2016

Uh oh!

yyscamper commented Feb 25, 2016

Uh oh!

srinia6 commented Feb 25, 2016

Uh oh!

yyscamper commented Feb 25, 2016

Uh oh!

yyscamper commented Feb 25, 2016

Uh oh!

srinia6 commented Feb 25, 2016

Uh oh!

srinia6 commented Feb 25, 2016

Uh oh!

yyscamper commented Feb 25, 2016

Uh oh!

srinia6 commented Feb 25, 2016

Uh oh!

jlongever commented Feb 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants