Update middleware.test.js to take loopback#rest into consideration #114
Conversation
| @@ -91,7 +91,7 @@ describe('loopback:middleware generator', function() { | |||
| var newSources = Object.keys( | |||
| readMiddlewaresJsonSync('server')['routes:after']); | |||
| var expectedSources = builtinSources.concat(['my-middleware-4']); | |||
| expect(newSources).to.have.members(expectedSources); | |||
| expect(expectedSources).to.have.include.members(newSources); | |||
There was a problem hiding this comment.
This looks rather weird to me. The argument of expect() is always the actual value as observed by the test, not the value we are expecting to see. Why is this change needed?
There was a problem hiding this comment.
newSources refers to an array with the name of a single middleware - my-middleware-4, which is added for this test.
expectedSources refers to an array with the default loopback#rest and my-middleware-4, which was added for the test.
How do you suggest this assertion be composed?
There was a problem hiding this comment.
You should fix L88-L89 and set builtinSource to the content of routes:after phase instead of current routes phase. After that, expectedSources should contain both default loopback#rest and added my-middleware-4.
There was a problem hiding this comment.
To be honest, I am confused - it seems that the CI build is passing this test even with the new version of loopback-workspace after strongloop/loopback-workspace#230 was landed.
|
Closing in favour of #115. |
loopback#restis expected to be present by default because of strongloop/loopback-workspace#230