-
Notifications
You must be signed in to change notification settings - Fork 285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(cmd-api-server): fix flaky runtime-plugin-imports-test #1903
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leeyoungone Great investigation I'm pretty sure you are on the right track. I read the documentation of node-cleanup and the way you are using it now seems to be a no-op to me because the purpose of the library is to provide hooks where the user (who is the person writing the Cactus code in this case) can do the "cleanup" logic themselves if they know that some allocated resource (TCP socket, file handle, database lock, etc.) is still being held onto. The point is, the node-cleanup library doesn't magically know what to clean up, it just gives you the opportunity to do so yourself (and right now what you technically do when you invoke the library's function is just saying that you want do the cleanup, but then you aren't).
So in that sense, I think this warrants a little more investigation, but you are definitely on the right track!
3628838
to
bb3b4d7
Compare
@petermetz Thank you so much for the advice! I've dug into it a little more and I think that adding the |
e9332c9
to
133d6a1
Compare
4c2c21a
to
125625f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leeyoungone Hmm, yarn.lock is still in diff if you look here: https://github.com/hyperledger/cactus/pull/1903/files
Did you forget to push your changes maybe?
5a91aa3
to
0f43596
Compare
4006281
to
b9de425
Compare
36e1bc4
to
4d1c26c
Compare
Migrated test from Tap to Jest. File Path: packages/cactus-test-cmd-api-server/src/test/typescript/integration/runtime-plugin-imports.test.ts ======================================================================= This is a flaky test case that can't be reliably reproduce. I've ran the test individually 40 times and all 40 times it has passed successfully. This error, however, seems to pop up only when the `yarn jest` command is run (when I ran the test individually). Based on the linked ["hint"](https://stackoverflow.com/a/50793993) from the issue, it seems like using `jest.useFakeTimers()` a good solution. However, using the `jest.useFakeTimers()` with`async/await` was not recommended. However, I found a [different source] (https://gist.github.com/apieceofbart/e6dea8d884d29cf88cdb54ef14ddbcc4#file-test-js-L58) that showed examples of how to use the `jest.useFakeTimers()`. Our original problem, however, is probably because the after the test environment is torn down, the second test file is trying to import files from the first environment, triggering the error ([explained here](jestjs/jest#11438 (comment))). So I've come to the solution of installing another node package: [node-cleanup](https://www.npmjs.com/package/node-cleanup) and running that within the afterAll at the very end of the test. My laptop can't take running the `yarn jest` script too much so I haven't seen it reproduce the error yet. (WOT ruhrow) _______________________edit below _________________________ So it turns out that this flaky test due to libraries trying to third libraries trying to run after the test runs is a simple fix that was documented [here](https://testing-library.com/docs/using-fake-timers/) where we just add the `useRealTimer` in the `afterEach`. Lol so sorry!! ======================================================================= This is a PARTIAL resolution to issue hyperledger-cacti#238 Fixes hyperledger-cacti#1667 Signed-off-by: awadhana <[email protected]> Signed-off-by: Youngone Lee <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leeyoungone LGTM, thank you!
This is a flaky test case that can't be reliably reproduce. I've ran the test individually 40 times and all 40 times it has passed successfully.
This error, however, seems to pop up only when the
yarn jest
command is run (when I ran the test individually).Based on the linked "hint" from the issue, it seems like using
jest.useFakeTimers()
a good solution. However, using thejest.useFakeTimers()
withasync/await
was not recommended.However, I found a different source that showed examples of how to use the
jest.useFakeTimers()
. Our original problem, however, is probably because the after the test environment is torn down, the second test file is trying to import files from the first environment, triggering the error (explained here). So I've come to the solution of installing another node package: node-cleanup and running that within the afterAll at the very end of the test.My laptop can't take running the
yarn jest
script too much so I haven't seen it reproduce the error yet. (WOT ruhrow)_______________________edit below _________________________
So it turns out that this flaky test due to libraries trying to third libraries trying to run after the test runs is a simple fix that was documented here where we just add the
useRealTimer
in theafterEach
. Lol so sorry!!Fixes #1667