-
Notifications
You must be signed in to change notification settings - Fork 41
Run unit tests in CI as a separate job #1562
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
base: develop
Are you sure you want to change the base?
Conversation
OCaml Reference Validation ResultsRepository: https://github.com/MinaProtocol/mina.git Click to see full validation output |
✓ Code Reference Verification PassedAll code references in the documentation have been verified successfully! Total references checked: 1 The documentation is in sync with the codebase on the |
.github/workflows/build-ci.yaml
Outdated
| runs-on: ${{ inputs.os }} | ||
| strategy: | ||
| matrix: | ||
| os: ${{ github.event.inputs.os && fromJSON(format('["{0}"]', github.event.inputs.os)) || fromJSON('["ubuntu-22.04", "ubuntu-24.04", "ubuntu-24.04-arm", "macos-13", "macos-14", "macos-15", "macos-latest"]') }} |
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.
This line has been repeated several time. I'll see if there's a better way that we can do this
.github/workflows/frontend-ci.yaml
Outdated
| build: | ||
| strategy: | ||
| matrix: | ||
| os: ${{ github.event.inputs.os && fromJSON(format('["{0}"]', github.event.inputs.os)) || fromJSON('["ubuntu-22.04", "ubuntu-24.04-arm", "macos-13", "macos-14", "macos-15", "macos-latest"]') }} |
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.
Same comment as above
.github/workflows/build-ci.yaml
Outdated
| @@ -1,32 +1,19 @@ | |||
| name: Reusable Build Workflow | |||
| name: Build CI | |||
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.
The idea of having separated files is to have different badges to add in the README: https://github.com/o1-labs/mina-rust?tab=readme-ov-file#supported-platforms. We can therefore see which platform is supported and the ones that are not.
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.
Wow, that really doesn't make sense that checks in a PR work on a job-by-job basis but status badges only work on a workflow by workflow basis. Let me see if there's another way to do this. It's a lot harder to get a complete picture of what's going on when only looking at the code if you have a bunch of different workflows all with the exact same trigger
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.
Actually, we can just use shields.io, it's free
https://img.shields.io/github/check-runs/o1-labs/mina-rust/rp/test-everything?nameFilter=deploy
I haven't quite figured out the naming (deploy works, but I couldn't figure out the others), but I really don't think having a bunch of separate workflows is worth it just for the pretty badges
2977f7e to
2f34ea5
Compare
5f40c78 to
54023b8
Compare
|
... I hope this one worked... |
108e0f0 to
2b06455
Compare
|
|
||
| - name: Run unit tests | ||
| run: | | ||
| cargo nextest run --workspace --lib \ |
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.
Could we have a target in the Makefile for this instead, please? We try to have all the commands run by the CI in the Makefile.
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.
I added a TODO here. I want the changes to be apparent here for this specific job. Once we remove all the exclude flags, I think we can add this to the makefile
| use wasm_bindgen_test::wasm_bindgen_test as test; | ||
|
|
||
| #[test] | ||
| #[ignore = "the file are missing"] |
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.
2b06455 to
644cd16
Compare
This PR starts running unit-tests, just for
mina-treefor now.We can start commenting out the "exclude" flags one by one as we enable more and more tests.