test(ci): require some core tests #2966
Comments
outSH
added
enhancement
|
@outSH Thank you for keeping track of these! Most of them are now enabled and set as required so we are making great progress, BUT there is one that I'm still looking into: the cactus-core pkg's tests for some reason pass fine on my local environment but fail in the CI and without any kind of error logged to the console. I'm suspicious that it has to do with the seg fault errors that we've been getting from Jest due to ESM problems, but those were always reproducible locally as well so I'm still investigating. Any ideas are most welcome as well! |
|
@petermetz Yeah, I've noticed this some time ago too and it indeed works flawlessly on my machine as well (even though I use roguhly exact same setup as CI!). This test was passing when I've created the list, I think it started failing after https://github.com/hyperledger/cacti/commit/d70488a82e9c1d6958ac3ab0368f3c3bfca378c6#diff-3dc426ffa27f30f292289225a0e941ab0406286a34d200e597748fce0414a90c but I also have no idea what is causing it, ESM problem seem possible |
|
@outSH That link seems like a solid clue, thank you very much! I'll focus my investigation on that file and the code that it tests! |
The fix was to start statically importing the http helper library we use in `packages/cactus-core/src/main/typescript/web-services/handle-rest-endpoint-exception.ts` instead of how it was (dynamic imports at runtime). 1. There have been reports of dynamic imports causing segmentation faults in the NodeJS process when Jest is involved. 2. It is looking like this bug was another instance of that manifesting but slightly differently because this time around Jest decided to hide the stack trace of it as well. 3. Because of `2)` we have no specific evidence of the theory. We can only say that the change in this commit made the problem go away, but since there never was any crash logs or stack traces on the CI environment, this remains a conjecture. 4. Trying to reproduce the issue locally failed even when using the exact same versions of NodeJS and npm (and all the dependencies of course). 5. Based on `4)` it is likely that the segmentation fault is due to a race condition in the lower level (C/C++) code of NodeJS and/or Jest. Fixes hyperledger-cacti#2966 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
This change makes it easier to read and debug some of the assertions of the test case which are a little convoluted due to the usage of the Jest spies. These changes were done as part of another fix but ultimately turned out to be not a factor for that fix and therefore I thought it bess to separate the change out onto it's own PR to make it easier to review. Related to hyperledger-cacti#2966 but this is not the actual fix. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
|
@outSH I've just opened the PR to finish the last one. I can't prove it 100% but I think it's still Jest doing seg faults when we have dynamic imports in the code. My theory is that it only manifests on low core count CPUs with less parallelism and that's why we can't reproduce it at all on local machines. |
The fix was to start statically importing the http helper library we use in `packages/cactus-core/src/main/typescript/web-services/handle-rest-endpoint-exception.ts` instead of how it was (dynamic imports at runtime). 1. There have been reports of dynamic imports causing segmentation faults in the NodeJS process when Jest is involved. 2. It is looking like this bug was another instance of that manifesting but slightly differently because this time around Jest decided to hide the stack trace of it as well. 3. Because of `2)` we have no specific evidence of the theory. We can only say that the change in this commit made the problem go away, but since there never was any crash logs or stack traces on the CI environment, this remains a conjecture. 4. Trying to reproduce the issue locally failed even when using the exact same versions of NodeJS and npm (and all the dependencies of course). 5. Based on `4)` it is likely that the segmentation fault is due to a race condition in the lower level (C/C++) code of NodeJS and/or Jest. Fixes hyperledger-cacti#2966 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
The fix was to start statically importing the http helper library we use in `packages/cactus-core/src/main/typescript/web-services/handle-rest-endpoint-exception.ts` instead of how it was (dynamic imports at runtime). 1. There have been reports of dynamic imports causing segmentation faults in the NodeJS process when Jest is involved. 2. It is looking like this bug was another instance of that manifesting but slightly differently because this time around Jest decided to hide the stack trace of it as well. 3. Because of `2)` we have no specific evidence of the theory. We can only say that the change in this commit made the problem go away, but since there never was any crash logs or stack traces on the CI environment, this remains a conjecture. 4. Trying to reproduce the issue locally failed even when using the exact same versions of NodeJS and npm (and all the dependencies of course). 5. Based on `4)` it is likely that the segmentation fault is due to a race condition in the lower level (C/C++) code of NodeJS and/or Jest. Fixes #2966 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
This change makes it easier to read and debug some of the assertions of the test case which are a little convoluted due to the usage of the Jest spies. These changes were done as part of another fix but ultimately turned out to be not a factor for that fix and therefore I thought it bess to separate the change out onto it's own PR to make it easier to review. Related to hyperledger-cacti#2966 but this is not the actual fix. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
This change makes it easier to read and debug some of the assertions of the test case which are a little convoluted due to the usage of the Jest spies. These changes were done as part of another fix but ultimately turned out to be not a factor for that fix and therefore I thought it bess to separate the change out onto it's own PR to make it easier to review. Related to #2966 but this is not the actual fix. Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
I'd like to require certain tests to always pass before PR can be merged. The list is small to require only tests with no false-nagative results and no complex setup (i.e. no functional test suites). My proposition (in addition to already enabled weaver tests) would be:
@petermetz @takeutak @izuru0 @jagpreetsinghsasan @VRamakrishna @sandeepnRES please let me know what you think.
The text was updated successfully, but these errors were encountered: