Skip to content
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

include wasm-test.sh in test-ci-all (almost) #4474

Merged
merged 4 commits into from
Mar 8, 2024

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Mar 6, 2024

This required:

  • reachitecting flakebox target vs toolchain handling
  • fixing wasm-test.sh to stop invalidating ./target
  • bunch of nix glue to stich native vs wasm32-unknown ./targets together
  • continuously ignoring the question is it even worth the time

Unfortunately after tackling all the scripts & Nix issues, it turned out that wasm-tests (headless firefox?) just doesn't work when being run in GNU parallel or xargs -P, and debugging it seems like lots of work.

So we'll still run wasm-test separately, but at least it will now re-use build artifacts with the main build, so it should still be faster. And in the future we can try again.

@dpc dpc force-pushed the 24-02-20-run-test-in-ci-all branch from 69ab8ce to 9f36a10 Compare March 6, 2024 07:49
@dpc
Copy link
Contributor Author

dpc commented Mar 6, 2024

@maan2003

Any idea why this test might be failing now?

It seems like it hangs and timeout kills it. The same thing happens locally on my machine, so I doubt it's running out of resources.

nix build -L .#wasm32-unknown.ci.wasmTest

uses the same method and works perfectly.

Seems like two rust tests in that hanged wasm-test are actualy passing so everything looks like it's working.

@maan2003
Copy link
Member

maan2003 commented Mar 6, 2024

very weird I have no idea why it would hang

@dpc dpc force-pushed the 24-02-20-run-test-in-ci-all branch from 9f36a10 to 65c72c2 Compare March 7, 2024 03:07
@dpc dpc changed the title include wasm-test.sh in test-ci-all include wasm-test.sh in test-ci-all (almost) Mar 7, 2024
@dpc dpc marked this pull request as ready for review March 7, 2024 04:40
@dpc dpc requested review from a team as code owners March 7, 2024 04:40
@dpc dpc marked this pull request as draft March 7, 2024 04:49
@dpc dpc force-pushed the 24-02-20-run-test-in-ci-all branch from 65c72c2 to d70a081 Compare March 7, 2024 04:51
@dpc dpc marked this pull request as ready for review March 7, 2024 05:20
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last commit undoes a lot of the second to last one. I think you can squash them to just keep the changes that actually worked.

nix/flakebox.nix Outdated Show resolved Hide resolved
fedimint-client/src/envs.rs Outdated Show resolved Hide resolved
nix/flakebox.nix Outdated Show resolved Hide resolved
@dpc dpc enabled auto-merge March 7, 2024 16:37
@dpc
Copy link
Contributor Author

dpc commented Mar 7, 2024

The last commit undoes a lot of the second to last one.

Yeah, I want these two separately because I want to be able to revert it when time comes to try again. It's a workaround for a blocker, ideally it would not be there at all. :(

@dpc dpc force-pushed the 24-02-20-run-test-in-ci-all branch from d70a081 to ca9bf08 Compare March 8, 2024 15:11
@dpc dpc requested a review from elsirion March 8, 2024 15:12
Copy link
Member

@Kodylow Kodylow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, issue created for TODO in test-ci-all

@dpc dpc added this pull request to the merge queue Mar 8, 2024
Merged via the queue into fedimint:master with commit 0a1474f Mar 8, 2024
20 checks passed
@dpc dpc deleted the 24-02-20-run-test-in-ci-all branch March 8, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants