Feat add exclude pattern running tests #2361
Conversation
julienbrs
requested review from
cptartur,
integraledelebesgue,
ksew1 and
tomek0123456789
as code owners
|
Please fill the checklist and add your changes to the changelog :) |
|
Also, always make sure to run |
|
Hi! This pull request hasn't had any activity for a while, so I am |
kkawula
left a comment
kkawula
left a comment
There was a problem hiding this comment.
Please run cargo lint before committing changes and address CI run errors.
|
Hi @julienbrs, any updates here? |
|
Will fix it asap tonight or next day |
|
Hi @julienbrs how's it going? |
|
Hi @kkawula, everything is working correctly except for one failing test: Expected output (from the documentation snippet): Actual Output from the test runner: Why I think it's happeningI suspect the issue is due to how --exit-first is currently implemented. When a test fails, the runner sets interrupted = true and stops processing new tests, but any tests that were already spawned asynchronously still run to completion (afaik). |
cptartur
removed request for
cptartur,
ddoktorski,
integraledelebesgue,
ksew1 and
tomek0123456789
|
@kkawula please request some from the team once it's read for the second review. |
It isn't related to your changes directly, your conclusion is probably correct. Could you please add there a few tests, to show that they are |
|
Hi @julienbrs, any progress on this? |
…pattern-running-tests
My bad, I might have made an autoformat on that md file and it added an empty line between that snippet code and the above Apart from that, I think everything is fine now |
kkawula
left a comment
kkawula
left a comment
There was a problem hiding this comment.
We have added more tests to the master branch, that's why your CI is failing, you should add 0 excluded entry in these tests. Also, our CI now uses rust 1.86 for formatting and limiting. If you encounter any problem please update your local Rust to stable.
| #[test] | ||
| fn with_exclude_flag() { | ||
| let temp = setup_package("simple_package"); | ||
| // Exclude the test "test_failing" |
There was a problem hiding this comment.
| // Exclude the test "test_failing" |
| .arg("--exclude") | ||
| .arg("test_failing") | ||
| .assert() | ||
| .code(1); // Assuming no other test fails |
There was a problem hiding this comment.
| .code(1); // Assuming no other test fails | |
| .code(0); |
| .arg("--exclude") | ||
| .arg("test_failing") |
There was a problem hiding this comment.
excluding all failed tast should be better
kkawula
left a comment
kkawula
left a comment
There was a problem hiding this comment.
HI @julienbrs, do you have further plans to work on this? After a little talk with the team, we decided that this flag should be renamed to --skip to stay consistent with cargo test
Hi @kkawula, will have some time to work + finish this PR for the next coming days. the --skip change is noted! |
|
@julienbrs It is also worth mentioning that we would like to mimic |
|
Hi @kkawula, I'm really sorry, but I won't be able to dedicate time to it after all. Things have become busier than expected on my end. Feel free to continue or reassign it as needed. Thanks for your understanding, and sorry again for the inconvenience. |
|
Okey, we understand and appreciate your contribution. We will take care of this pr and award you when it is merged. |
4 participants
Closes #1512
Introduced changes
--excludeflag that functions similarly to the existingFilterName. This flag allows users to exclude tests based on their names.snforge test simple_testlaunches all tests beginning with "simple_test", rather than requiringsnforge test --filter simple_test). Given this, I believe the new--excludeoption might not be compatible with the existing filter option. What do you think?Checklist
CHANGELOG.md