fixing issue #259 and unit test cleanup #346
Conversation
|
|
test this please |
|
+1 |
|
@yyscamper not sure if you want to take a look at this fix since it addresses github issue RackHD/RackHD#259 that you worked on |
| var _ = injector.get('_'); | ||
|
|
||
| module.exports = function create(fittingDef) { | ||
| injector = require('../../index.js').injector; |
There was a problem hiding this comment.
line #5 has the same code, what's the difference? Could you please explain more why this is the root case?
There was a problem hiding this comment.
The difference is in workflows we are using simpleWrapper to mock an injectable and what's happening is the swagger_render takes this injectable and exports the function with it. The Profiles test then continues to use this "stale" injectable.
By adding in this line we are essentially refreshing the injectable to make sure if there were any changes the exported function takes them into account.
@zyoung51 can correct me if my understanding is wrong or if he wants to add more :)
There was a problem hiding this comment.
But I know the require has cache (see https://nodejs.org/api/modules.html#modules_caching), so the two require operation should return the same object, I think this line will not refresh the injector
There was a problem hiding this comment.
@yyscamper This change is a work-around for an issue with the way mocking is being done. Without this line, the http server will run with an old version of the injector that still has mocks from the previous test. We have work planned to make structural changes to all of the unit tests to avoid the need for this work-around in the future.
There was a problem hiding this comment.
@brianparry : thanks for your explanation. I just wondering how this line of code works, because of the require caching, normally you should not get a new version of injector. But depends on your testing, this should work. Maybe some of my understanding is not correct.
There was a problem hiding this comment.
@yyscamper I'm not entirely sure why this works. For some reason there is a difference in the injector object between initially requiring the fitting and loading the fitting. Given that this is a temporary work-around for a test-only problem, I think it's acceptable for now.
|
@BillyAbildgaard thanks for your fix. Could you please clarify my above questions? and also the following question: |
|
@yyscamper I ran the tests in general and ran the specific case you saw where running workflows before profiles caused the failure. The fix eliminated this case. This story was just to fix the bug. Stryker team has an additional story in our backlog to look into running the unit tests randomly |
|
@BillyAbildgaard : The whole solution of random unit-test will need relative much consideration, but if you want to try, so can simplify use Could you please try above random testing with enough times? Because I think that issue should not be considered as fully resolved if the random testing fails. |
|
@yyscamper I can confirm the fix does resolve the bug that was reported. I tested it multiple times. We have a story for randomized testing, but that was outside the scope of what I was trying to do for this, which was just to resolve the bug reported in RackHD/RackHD#259. |
|
@yyscamper As @BillyAbildgaard has mentioned, this PR is a general cleanup of unit test mocking and a fix for RackHD/RackHD#259. Full random-order testing support is planned as part of a future story and is outside the scope of this PR. |
| configuration = helper.injector.get('Services.Configuration'); | ||
| sinon.stub(configuration, 'set').returns(configuration); | ||
| sinon.stub(configuration, 'getAll').returns({}); | ||
| this.timeout(10000); |
There was a problem hiding this comment.
Does this test fail consistently without the longer timeout?
There was a problem hiding this comment.
@brianparry not that I saw, but I figured I would just increase the timeout since I was in there to decrease the likelihood of a failure because of a timeout.
|
@BillyAbildgaard @brianparry : I agree on the scope for this PR. So if there is any more unit-test error if run in random order, we can fix it then. |
5 participants
resolves https://github.com/RackHD/RackHD/issues/259
The actual fix for the bug was the line I added to the swagger_render. @zyoung51 helped me find that fix.
@RackHD/corecommitters @dalebremner @srinia6 @brianparry