Extend model from D15557 to all tests and setup scripts
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABCb5103bab634b: [chronik-client] Improve and standardize integration tests
CI
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
Comment Actions
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
Comment Actions
not sure what to make of this. build log seems to be failing in different way vs before but this is just from memory
Comment Actions
not sure what to make of this. build log seems to be failing in different way vs before but this is just from memory
That's still the same issue, so the cause is something else (or there are several causes).
| modules/chronik-client/test/integration/block_and_blocks.ts | ||
|---|---|---|
| 89 ↗ | (On Diff #45765) | Can we move all this out of the tests ? So we don't have to copy paste it every time |
Comment Actions
This still need to be factorized so we don't repeat the same code in all tests, but otherwise lgtm
Comment Actions
Standardize before and after methods with functions.
The whole block is still duplicated, despite the function. All the setup from launching the child process to getting the test info and setting the timeout should be moved out of the test. Can it be part of initialize_test_runner?
Comment Actions
I ran into weird errors related to variable passing and types when I tried to factor out the testRunner.onMessage stuff. It is maybe possible to return testInfo from the function, but because this comes from the testRunner.onMessage routine I did not want to go down that rabbit hole again.
As for not duplicating the before and after routines. There probably is some way to do this. I have not been able to immediately find a ready solution though, internet seems to recommend writing custom wrappers, which I would like to avoid (at any rate, the scope of the problem is such that I think it should be in another diff, it's not as simple as it seems it should be, or I haven't been able to figure it out in this way).
imo there may be more room for optimization here, but it's to the point where ROI of fighting the mocha syntax is not worth the expected benefit. I get that if it doesn't happen here, it might not ever happen -- but the diff is still a significant improvement and patches an existing bug. Adding more tests is not unduly complicated with many existing files of the same pattern.
Preference would be to land as-is here and, if I come across a way to do this while constantly living and fighting with mocha in many repos, implement it. At the moment this diff is a blocker for finishing up the rest of the in-node features to chronik-client.
| modules/chronik-client/test/integration/block_and_blocks.ts | ||
|---|---|---|
| 89 ↗ | (On Diff #45765) | we can make the logic into a function pushing the done event test to CI first to see if it works |
Comment Actions
Fair enough, let's the factorization happen in a follow up and keep only the proper child process shutdown for this (large enough) diff.