-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
test.py: Make it possible to avoid wildcard test names matching #18704
Conversation
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.
Please rewrite the patch title and commit message to use correct terminology - you used the phrases "test case matching" and "test name", where I think in both cases you really mean the test file's name which is being match, not the test "case", but to be honest I don't know exactly what you meant which is why I'm askng to clarify the text.
My second question is the following: The current behavior is not accidental: @kostja who initially wrote this code really wanted people to be able to write "test.py lwt" and run multiple tests whose file name has the word "lwt" in it, in multiple suites. I was never a fan of this idea (I much prefer the pytest way of doing things), but before we drop it we need to understand if people no longer want this ability.
If people still want this ability, we could implement both - e.g., add an option like "--substring" to enable substring match (disabled by default) or "--exact" to disable substring match (enabled by default) - and then CI could use the option it wants to use, and users like @kostja can use the option they want to use.
a4679f4
to
a23eccd
Compare
Done |
Absolutely
Another option is to make exact single test file selection with the help of |
I think that's a good idea - any name that contains "::" should be considered just for an exact match and not for pattern matching - regardless of how we handle names without "::". I think this would be good enough for the CI usage you mentioned, but to be honest, I'm not familiar with that code and if it is aware of suite names or just file names. |
@yaronkaikov , would that work for CI? |
I assume it will need some adjustment but it could work |
There's a nasty scenario when this searching plays bad joke. When CI picks up a new branch and notices, that a test had changed, it spawns a custom job with test.py --repeat 100 $changed_test_name in it. Next, when the test.py tries opt-in test name matching, it uses the wildcard search and can pick up extra unwanted tests into the run. To solve this, the case-selection syntax is extended. Now if the caller specifies `suite/test::*` as test, the test file is selected by exact name match, but the specific test-case is not selected, the `*` makes it run all cases. Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
a23eccd
to
c4d4578
Compare
upd:
|
Hmm, now that you (sort of...) clarified where you meant test file ("test") and where you meant test function ("case"), I have to wonder - you write |
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 approve this PR, because forgoing all the philsophical discussions, what it really amounts to is a single line allowing the test-case name "*" to mean all tests in that file - and all the rest of the philosophy can be moved aside.
But I'm still worried that we keep complicating the test specification more and more for no reason. I think eventually we should switch to pytest's syntax and semantics (no substring search) and stop inventing our own.
@@ -38,10 +38,17 @@ If you want to run only a specific test: | |||
|
|||
$ ./test.py suitename/testname | |||
|
|||
If you want to run only a specific test-case from a test: | |||
This will select and run all tests having suitename/testname as substring, | |||
e.g. suitename/testname_ext and others. |
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.
So in this case, it's not really a substring, it's a prefix? Would suitename/hello_testname
also match, or not?
@kostja @kbr-scylla and others - please consider if this is what you really wanted to happen. I think if you provide a "full path" of the test - suite and test file - then it should be an exact match, not a substring or prefix match. I think the substring should only be done when just a single word (e.g., "lwt") is given. But maybe I am missing the original intent of this feature?
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.
It's a substring, the check is if patter in testname
, so suite/foo wouldn't match suite/ext_foo, but plain foo would
@kostja @kbr-scylla because I know you don't like to see test.py "decay" with random changes in the wrong (in your view) direction without a discussion - I'd like to give you guys the opportunity to review this (fairly trivial) patch before merging it, despite getting three approvals already. |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
🟢 CI State: SUCCESS✅ - Build Build Details:
|
I am not using the system as much as you guys now-a-days. If you feel the new UX is more convenient - go with it. I think it's a non-issue, because you can always provide a full path to the test file, thus effectively avoid any wild-carding. |
Another way to avoid wildcarding is to add .py at the end. but this of course got broken with the (incomplete) addition of test case support, right @xemul |
Long term @temichus is convincing me that every test should be a pytest, including unittests, and we should kill test.py in favour of pytest being the main driver. I tend to give in - after all it's a UX one will be less nit-picky about. |
🟢 CI State: SUCCESS✅ - Build Build Details:
|
Did it ever worked? I didn't change the script workflow in when test case is not selected |
I don't think it ever did. Commit 1955f91 that introduced Python suite:
|
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.
Qd
There's a nasty scenario when this searching plays bad joke.
When CI picks up a new branch and notices, that a test had changed, it spawns a custom job with
test.py --repeat 100 $changed_test_name
in it. Next, when the test.py tries opt-in test name matching, it uses the wildcard search and can pick up extra unwanted tests into the run.To solve this, the case-selection syntax is extended. Now if the caller specifies
suite/test::*
as test, the test file is selected by exact name match, but the specific test-case is not selected, the*
makes it run all cases.