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

tests: disable rewrite_solve_xyz_parameterized by default #572

Closed
wants to merge 2 commits into from

Conversation

niklasdewally
Copy link
Collaborator

@niklasdewally niklasdewally commented Jan 8, 2025

The rewrite_solve_xyz_paramterized test case is relatively slow, so disable it by default. This significantly improves the time taken to run cargo test locally.

The Rust book recommends using #[ignore] to disable expensive tests by default 1. These can be re-enabled using cargo test -- --ignored / cargo test -- --including-ignored.

Add quick and all parameters to the test CI to run both default tests and all tests (including ignored ones).

@niklasdewally niklasdewally self-assigned this Jan 8, 2025
@niklasdewally niklasdewally force-pushed the nik/pr/disable-rewrite-parametric-test branch from dab5b76 to 5290bc1 Compare January 8, 2025 13:20
@niklasdewally niklasdewally changed the title WIP tests: disable rewrite_solve_xyz_parameterized by default Jan 8, 2025
The rewrite_solve_xyz_paramterized test case is relatively slow, so
disable it by default. This significantly improves the time taken to run
`cargo test` locally.

The Rust book recommends using #[ignore] to disable expensive tests by
default [1]. These can be re-enabled using `cargo test -- --ignored` /
`cargo test -- --including-ignored`.

Add `quick` and `all` parameters to the test CI to run both default
tests and all tests (including ignored ones).

[1]: https://doc.rust-lang.org/book/ch11-02-running-tests.html#ignoring-some-tests-unless-specifically-requested
@niklasdewally niklasdewally force-pushed the nik/pr/disable-rewrite-parametric-test branch from 5290bc1 to b8f0794 Compare January 8, 2025 13:21
@niklasdewally
Copy link
Collaborator Author

niklasdewally commented Jan 8, 2025

Alternatively we could remove this test case, don't think it does much?

@ozgurakgun
Copy link
Contributor

Happy to remove the test case.

Separately, I like the idea of having infrastructure to select which test to run depending on how expensive the test is though. In Conjure I optionally add a rough expected time (in seconds) for each test case. Then running --limit_time=5 runs only those tests that have an expected run time of <=5 seconds. Default value is 0.

Could be better than the coarse grained quick/all. Quick can then be an alias for <=1 or whatever and all can be actually all.

@EEJDempster @Soph1514 for info.

@niklasdewally
Copy link
Collaborator Author

Happy to remove the test case.

Separately, I like the idea of having infrastructure to select which test to run depending on how expensive the test is though. In Conjure I optionally add a rough expected time (in seconds) for each test case. Then running --limit_time=5 runs only those tests that have an expected run time of <=5 seconds. Default value is 0.

Could be better than the coarse grained quick/all. Quick can then be an alias for <=1 or whatever and all can be actually all.

@EEJDempster @Soph1514 for info.

Should I close this PR and delete the test case instead, or is this approach to quick/all fine for now?

niklasdewally added a commit that referenced this pull request Jan 8, 2025
As discussed in #572, this test case is relatively slow, and brings
little value, so we should remove it.

This improves `cargo test` run-times.
@niklasdewally
Copy link
Collaborator Author

niklasdewally commented Jan 8, 2025

PR #573 deletes this test case instead.

@ozgurakgun
Copy link
Contributor

Fine for now, we delete the test case and merge this as well. But can you please make an issue with the finer grained approach I write above?

@ozgurakgun
Copy link
Contributor

Actually maybe we don't need to merge this yet? Thanks for the issue.

@niklasdewally
Copy link
Collaborator Author

Documented in issue #574

@niklasdewally niklasdewally deleted the nik/pr/disable-rewrite-parametric-test branch January 10, 2025 20:24
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.

2 participants