fix(tests): split running remote and local tests #2623
Conversation
ghost
assigned
vbudhram
force-pushed
the
flake-1
branch
2 times, most recently
from
6014277 to
1ab3186
Compare
bin/key_server.js
Outdated
| op: 'promise.unhandledRejection', | ||
| error: reason | ||
| }) | ||
| process.exit(8) |
There was a problem hiding this comment.
We had this before in the auth-server, but had to remove it in prod because prod would get unhandledRejections
There was a problem hiding this comment.
This was added while narrowing down possible issues. Consider it gone!
vbudhram
force-pushed
the
flake-1
branch
2 times, most recently
from
ff7a723 to
24c963e
Compare
| return db.forgotPasswordVerified(passwordForgotToken) | ||
| .then(accountResetToken => { | ||
| assert.ok(accountResetToken.createdAt > passwordForgotToken.createdAt, 'account reset token should be newer than password forgot token') | ||
| assert.ok(accountResetToken.createdAt >= passwordForgotToken.createdAt, 'account reset token should be equal or newer than password forgot token') |
There was a problem hiding this comment.
This fixes #2534 (comment), where I think the reset token has the same timestamp as forgot token since it gets create right next to each other. I could have added a delay here but opted to just test that they were >= since a they could have same timestamp.
vbudhram
changed the title
|
@mozilla/fxa-devs No rush on this, r? |
philbooth
left a comment
philbooth
left a comment
There was a problem hiding this comment.
All looks fine to me. The module cache is weird, so I'm not surprised proxyquire can cause us problems. Presumably if we forced noPreserveCache() everywhere it would also get it working, but that's a much heavier change so I'm cool with doing this instead.
| "npmshrink": "1.0.1", | ||
| "nyc": "13.0.0", | ||
| "proxyquire": "1.8.0", | ||
| "proxyquire": "2.0.0", |
There was a problem hiding this comment.
Purely for my own curiosity, are there any cool new features/fixes we're getting with this upgrade or is it just to get us on the latest and greatest generally?
There was a problem hiding this comment.
I believe this upgrade just had bug fixes. I thought they might resolve some of the flaky test but didn't.
|
|
||
| let bin = NYC_BIN | ||
| let argv = ['--cache', MOCHA_BIN] | ||
| let argv = ['--cache', '--no-clean', MOCHA_BIN] |
There was a problem hiding this comment.
What does this do? (purely for my own curiosity, again)
There was a problem hiding this comment.
The --no-clean does not remove coverage files from previous runs. Since I split the test runs, it generates the coverage data as if it was run in one operation.
|
@philbooth Thanks for r!
Unfortunately, I tried that and didn't get much luck. A bunch of tests started failing for various reasons. Longer term maybe there is something better than proxyquire that we could use? |
Before I joined this team I always used to use mockery for the same purpose. But it's entirely possible that it would suffer from the same issue, they're both hacking the module cache at the end of the day. |
I wonder if proxyquire could be one source of troubles with the behavior I saw in @deeptibaghel's profile-server PR to update to Hapi 17, I commented over in another PR |
Another potential fix for #2534. This PR has successfully ran tests 5 and 10 times in a row, ref https://circleci.com/gh/mozilla/fxa-auth-server/3368.
I think part of the flaky test failures might involve
proxyrequireand caching modules. When running the local and remote test suites individually I could not get the majority of errors in #2534. I thought using proxyrequire withnoPreserveCache()andnoCallThru()would fix the issue but I could not determine the exact modules that needed them (it is probably very time consuming as well).To that end, this solution just splits remote and local tests and updates coverage tool to combine from both runs.