test.py: Make it possible to avoid wildcard test names matching

[xemul]

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.

[nyh]

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.

[xemul]

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.

Done

[xemul]

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.

Absolutely

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.

Another option is to make exact single test file selection with the help of suite/name::* syntax. In this case this ::* would mean that we want the specific test with specific test-case, but the specific test-case is * i.e. -- all test-cases

[nyh]

Another option is to make exact single test file selection with the help of suite/name::* syntax.

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.

[xemul]

Another option is to make exact single test file selection with the help of suite/name::* syntax.

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?

[yaronkaikov]

Another option is to make exact single test file selection with the help of suite/name::* syntax.

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

[xemul]

upd:

• extend the case-selection syntax to accept *
• updated docs

[nyh]

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.

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 suite/test::* to say that the word "test" is the whole test file name and not a substring. But... Why do we need that ::* and why isn't "suite/test" the same thing? If I understood correctly, the original test.py design (by @kostja and others) called for "test.py lwt" to run all the tests with the word lwt in their name - but if you explicitly write "test.py suite/lwt", do you also expect substring match? How or why?

[nyh]

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.

[nyh]

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?

[xemul]

It's a substring, the check is if patter in testname, so suite/foo wouldn't match suite/ext_foo, but plain foo would

[nyh]

@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.

[scylladb-promoter]

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - dtest with topology changes
✅ - dtest
✅ - Unit Tests

Build Details:

• Duration: 5 hr 0 min
• Builder: i-0d097b04649dda1b9 (m5ad.12xlarge)

[scylladb-promoter]

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - Unit Tests

Build Details:

• Duration: 3 hr 24 min
• Builder: spider7.cloudius-systems.com

[kostja]

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.

[kostja]

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

[kostja]

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.

[scylladb-promoter]

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - Unit Tests

Build Details:

• Duration: 7 hr 0 min
• Builder: i-01d1e280ad5a4ddef (r5ad.8xlarge)

[xemul]

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

Did it ever worked? I didn't change the script workflow in when test case is not selected

[xemul]

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

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:

$ ./test.py --list --mode=dev test_compaction_manager.py
Test test_compaction_manager.py not found
$ ./test.py --list --mode=dev test_compaction_manager
Found 1 tests.
rest_api/test_compaction_manager

[kbr-scylla]

Qd