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

[CI] Run proptests only on main #673

Merged
merged 7 commits into from
Jul 23, 2024
Merged

[CI] Run proptests only on main #673

merged 7 commits into from
Jul 23, 2024

Conversation

lubkoll
Copy link
Contributor

@lubkoll lubkoll commented Jul 18, 2024

No description provided.

@lubkoll lubkoll requested review from ajansari95 and magiodev July 18, 2024 20:16
@lubkoll lubkoll force-pushed the feat/cii-15 branch 3 times, most recently from 9c839c8 to b87219e Compare July 19, 2024 16:10
@0xLaurenzo 0xLaurenzo self-requested a review July 22, 2024 06:52
0xLaurenzo
0xLaurenzo previously approved these changes Jul 22, 2024
Copy link
Contributor

@0xLaurenzo 0xLaurenzo left a comment

Choose a reason for hiding this comment

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

lgtm

@0xLaurenzo 0xLaurenzo dismissed their stale review July 22, 2024 12:45

I misunderstood a part of the workflow

@0xLaurenzo 0xLaurenzo self-requested a review July 22, 2024 12:45
Copy link
Contributor

@0xLaurenzo 0xLaurenzo left a comment

Choose a reason for hiding this comment

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

The test-tube alias does not run the prop tests but is called by the workflow that should run the prop test

@@ -2,5 +2,6 @@
wasm = "build --release --lib --target wasm32-unknown-unknown"
unit-test = "test --lib"
schema = "run --bin schema"
test-tube = "test --package cl-vault --lib -- --include-ignored test_tube:: --nocapture --test-threads=1"
test-tube = "test --package cl-vault --lib -- --include-ignored test_tube:: --nocapture --test-threads=1 --skip test_complete_works"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't run the proptests. The --skip here skips the prop test, but the cl_vault.ym uses the rust-test-tube workflow which calls this command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly that is the feature:)
I.e. I'd like to not run the proptest when we call cargo test-tube, but only on cargo prop-test. The problem is that currently this one test takes most of the time in the pre-commit verification, but brings close to 0 value (didn't see any failure of it triggering any action since I am here -- and it fails regularly with the code even remotely touching anything related to it). Even if it fails it doesn't give you much directly actionable input.
My actual thoughts are that:

  • we should kill the proptests
  • else run it in a nightly job and just check if it is failing in the morning
  • first step is though to get it out of pre-commit verification

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, in that case I was confused by the PR title, since this PR effectively removes prop tests from the CI. Also lets discuss proptests later over slack or smth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching. That shouldn't be the case and running the proptest on main was part of this before I had to rebase 100x as there were no reviews.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added it back

@lubkoll lubkoll requested a review from 0xLaurenzo July 22, 2024 13:53
0xLaurenzo
0xLaurenzo previously approved these changes Jul 22, 2024
Copy link
Contributor

@0xLaurenzo 0xLaurenzo left a comment

Choose a reason for hiding this comment

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

lgtm, please make sure to change the PR title or the commit, so we clearly see this PR removes proptests from the CI

@lubkoll lubkoll force-pushed the feat/cii-15 branch 2 times, most recently from 0585e1e to c5e4b7b Compare July 22, 2024 14:16
@lubkoll lubkoll requested a review from 0xLaurenzo July 22, 2024 15:34
Copy link
Contributor

@ajansari95 ajansari95 left a comment

Choose a reason for hiding this comment

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

lgtm

@lubkoll lubkoll merged commit 6257fcb into main Jul 23, 2024
11 checks passed
@lubkoll lubkoll deleted the feat/cii-15 branch July 23, 2024 07:51
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.

3 participants