Run e2e in parallel using jest #2106
Conversation
|
Are you sure the changelog does not need updating? |
1 similar comment
|
Are you sure the changelog does not need updating? |
There was a problem hiding this comment.
the tests don't terminate
Are you executing all of them or just the new ones?
Remember that we did this stupid hack with mocha where we have to call run and stuff. Maybe that is interferring with jest.
Try deleting all other tests temporarily and see if that helps.
I need to manually delete logs/bitcoind after each run, otherwise the setup fails with:
Are we using mkdir -p? That shouldn't fail if the dir already exists.
| "noUnusedParameters": true, | ||
| "module": "commonjs", | ||
| "moduleResolution": "node", | ||
| "noEmit": false, |
There was a problem hiding this comment.
That should change back to noEmit true once you are done, otherwise the folder is always cluttered with the compiled JS files.
I guess you enabled it to compile the custom test environment.
If it is only for that, we can:
- limit the compilation to just this one file
- find out if there is a way to feed TS to jest for the test environment
- write the test env in JS instead of TS
There was a problem hiding this comment.
I would need help with this. If I set it to false then TestEnvironment is not transpiled.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The shutdown? I am leaning towars saying it is fine. Is there a big fat error message that might confuse people? |
|
Yes, the non-termination problem. When I run it with this it works: If I run it like this: I get the following output and it seems to never terminate: |
I just finished my task anyway. I'll checkout the PR and have a look. |
bonomat
force-pushed
the
1922-parallel-e2e
branch
from
8de5ad9 to
3761e4e
Compare
api_tests/package.json
Outdated
| "pretest": "cargo build --bin cnd", | ||
| "test": "ts-node ./harness.ts", | ||
| "pretest": "cargo build --bin cnd && tsc", | ||
| "test": "jest", |
| // Bitcoin/bitcoin Alpha Ledger/ Alpha Asset // | ||
| // Ethereum/ether Beta Ledger/Beta Asset // | ||
| // ******************************************** // | ||
| describe("Bitcoin/bitcoin - Ethereum/ether", () => { |
There was a problem hiding this comment.
The tests are pretty verbose now (again) with the move of it to the outside of the twoActorTest function. I like that we can now run them through the IDE but it would also be nice if you could try and consolidate some of those function calls again.
The actual test is 4 indentation levels in :(
There was a problem hiding this comment.
Mhm... just thinking out lout:
What if we start 2 cnd (or depending how many we need) per test suite/test file in a beforeAll block?
This might speed up our tests and stress test cnd 😬
(not sure if this is a good idea as long as we have flaky tests though... meaning forever)
bonomat
changed the title
This comment has been minimized.
This comment has been minimized.
|
Are you sure the changelog does not need updating? |
| actorNames: ["alice", "bob", "charlie"] | ["alice", "bob"] | ["alice"], | ||
| testFn: (actors: Actors) => Promise<void> | ||
| ) { | ||
| const name = JasmineSmacker.getCurrentTestName(); |
There was a problem hiding this comment.
cc @luckysori look what we are finally using :D
There was a problem hiding this comment.
I love the readme form JasmineSmacker :D
This is a very hacky solution to this issue on Jasmine's GitHub.
|
|
||
| // ******************************************** // | ||
| // RFC003 Swap Reject // | ||
| // ******************************************** // |
There was a problem hiding this comment.
I find this a bit odd but I guess you had your reason to add it :)
There was a problem hiding this comment.
I find it more readable? Happy to remove it if people don't like it :)
There was a problem hiding this comment.
I don't personally like these sort of comments, they do not add anything that the function names already add and are prone to becoming stale. I am always suspicious of codebases that use these style of comments, I actually tried to get these sort of comments removed from rustc (amusingly the issue got a thumbs up, a thumbs down, and a confused :) rust-lang/rust#62601
| ).to.be.greaterThan(1); | ||
| }); | ||
| }); | ||
| it("returns-links-to-create-swap-endpoints-on-root-document-as-siren", async function() { |
There was a problem hiding this comment.
Would be nice if we could just specify regular sentences here and we just replace the dashes with hyphens for the log file.
There was a problem hiding this comment.
Replace spaces and slashes with dashes*
There was a problem hiding this comment.
That'd be a lot of work. I prefer not doing it in this PR 😬
bonomat
force-pushed
the
1922-parallel-e2e
branch
from
0d9d8de to
e04e253
Compare
|
The last commit (9f0b8a9) is a bit questionable. It was an attempt to speed things up, i.e. parallelize stuff, but it seems like it has an opposite effect. //EDIT// seems to be only on my machine and circleCI likes it :) |
Looks fine to me :D Really good effort @bonomat ! Thanks for pushing through with this! It is a great foundation to build upon :) |
|
I don't know if this is included in one of your followup prs so I'll state it here again: Reorganize the test suites (i.e. files) per concern tested (happy, refund, etc) and then use it.each to loop through the assets/ledgers combinations. The suites should probably include the protocols as those determine, which actions you can take. |
Doing this allows us to use different settings for different e2e tests
Parallelize actors and wallet settings
bonomat
force-pushed
the
1922-parallel-e2e
branch
from
e04e253 to
c976953
Compare
|
@da-kami , @luckysori , @rishflab , @tcharding , @D4nte , @thomaseizinger : Please execute I propose to get this PR merged as rebasing is starting to get annoying 😶 Note: lnd tests are skipped in this version and I propose to tackle them separately in a follow-up PR. |
|
Runs on my machine |
|
Looks like we need |
|
|
From @rishflab : |
There was a problem hiding this comment.
passed on my machine
Output stats:
Test Suites: 1 passed, 1 total
Tests: 2 skipped, 20 passed, 22 total
Snapshots: 0 total
Time: 634.555s
Ran all test suites.
Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?
✨ Done in 708.52s.
|
Works on everyone's machine. I'll create some follow-up tickets. |
|
bors r+ |
1 similar comment
|
bors r+ |
|
Already running a review |
Build succeeded |
Once done,
resolves #1928
resolves #1927 (I'll create a follow-up ticket to run more tests in parallel see below. Note, running multiple tests using parity is hard because we would need a mutex around sending transaction -> nonce issue )
resolves #2137
99% review please:
My assumption were:
Ticket #1927 was partially resolved. Some tests are run in parallel but not all of them. Tests within 1 tests suite (in the same file) are not run in parallel. With this setup we can't split up all blockchain based e2e tests in multiple files though. However, I have an idea how this can be resolved. I'll create a follow-up ticket on this once this PR is merged. In short we need to do this:
Open TODOs
re-organise blockchain tests per concern tested (happy, refund, etc) and then use it.each to loop through the assets/ledgers combinations.
The suites should probably include the protocols as those determine, which actions you can take. --> Simplify e2e tests and reuse code #2190
create a follow-up ticket for parallelizing blockchain-based e2e tests: this can be quite tricky as we use 1 account for ethereum (parity-dev account) to fund each actor. --> Parallelize e2e tests and reuse code #2191
create a follow-up ticket for renaming e2e tests: test names are currently a bit messy and don't necessarily have to contain
-in the name. --> Rename e2e tests #2193fix LND-based sanity tests --> Fix LND-based sanity tests #2192