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

Wildcard support in launcher #3200

Merged
merged 4 commits into from Oct 2, 2022
Merged

Wildcard support in launcher #3200

merged 4 commits into from Oct 2, 2022

Conversation

nikunjy
Copy link
Contributor

@nikunjy nikunjy commented Sep 2, 2022

Supporting wildcard in kotest launcher based on the discussion here #2416 (comment)

I am extending the testpath to support wildcard. In the future we can support regexes with very small changes.

After this PR is merged you will be able to use the kotest-framework-engine-launcer with the following args

"--reporter", "teamcity", 
"--package", "com.kotlin.test",
"--spec", "*",
"--testpath", "db*",

The launcher already supports --testpath argument. And this PR is just extending that testpath option to be more flexible. It is written with the same reasoning in mind as the original --testpath arg.

Not only this but you would be able to use this to write a runner like this

 ExecuteKt.execute(
  reporter, 
  "<package name>",
  "<spec wildcard>"
  "<test wildcard>"
)

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 6, 2022

@sksamuel what do you think. Wanna give me a code review ? :)

@LeoColman
Copy link
Member

I think this PR is a duplicate of #3190. Do you think both have the same goal, @nikunjy

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 7, 2022

@LeoColman No. That one is very specific to gradle. Here is a comment by @sksamuel
#2416 (comment)

I'm happy to expand the launcher but I'd rather do it better than the
gradle way (we just do that for compatibility).
Perhaps accept a wildcard for --spec and --test or something ?

It remains to be seen if what I am doing here is the "better" way. But I am motivated to push this to completion. :) Happy to accomodate any comments and revisions.

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 8, 2022

@LeoColman one of my tests failed. I fixed it, it requires your approval to run these tests. Can you please retrigger them, sorry about that. Also I notice a lot of tests in this repo get skipped, is this expected ?

@jschneidereit
Copy link
Member

I'm a little confused about what we're doing here - I'm pretty sure most of this functionality exists in these two files:

kotest-framework/kotest-framework-engine/src/commonMain/kotlin/io/kotest/engine/spec/interceptor/SystemPropertySpecFilterInterceptor.kt

and

kotest-framework/kotest-framework-engine/src/commonMain/kotlin/io/kotest/engine/test/status/SystemPropertyTestFilterEnabledExtension.kt

all you should have to do iirc is set the sysprop/envvar appropriately before starting your runner

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 8, 2022

@jschneidereit If this PR is merged you will be able to use the kotest-framework-engine-launcer with the following args

"--reporter", "teamcity", 
"--package", "com.kotlin.test",
"--spec", "*",
"--testpath", "db*",

The launcher already supports --testpath argument. And this PR is just extending that testpath option to be more flexible. It is written with the same reasoning in mind as the original --testpath arg.

Not only this but you would be able to use this to write a runner like this

 ExecuteKt.execute(
  reporter, 
  "<package name>",
  "<spec wildcard>"
  "<test wildcard>"
)

There is some conversation in here #2416 as well

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 8, 2022

Changed the description. Thanks for asking @jschneidereit I should added more description in the PR

@LeoColman
Copy link
Member

I approved the run again.

No. That one is very specific to gradle

I stand corrected, I overlooked the "wildcard" keyword only :P

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 8, 2022

Looks like the tests have passed. What are the next steps @LeoColman

@jschneidereit
Copy link
Member

What I was getting at is I think this is a duplicate implementation

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 8, 2022

@jschneidereit can you elaborate. I highlighted the reasons on why it is different.
Furthermore the motivation came from conversation with the author @sksamuel
He asked me to extend the launcher jar and in a different way than project does for gradle.

You can target test, subtest etc by providing the right parameters --testpath foo * -- bar* . Can you help me understand why you still think is duplicate.

@sksamuel
Copy link
Member

sksamuel commented Sep 8, 2022

I don't think the existing stuff supports the wildcard @jschneidereit ?

@jschneidereit
Copy link
Member

I'm probably misunderstanding something here, but I recall adding test filtering by regex here:

https://github.com/kotest/kotest/pull/2547/files#diff-c8b53629477f4bcb438b5764976c6fc1d2999cf499bf53a7b062fdb986531de4R106

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 8, 2022

Would that approach apply if I am using the fat jar kotest-framework-engine-launcer binary (written here). If yes then I guess my question is why does the it take --testpath or --spec at all in that case.

@jschneidereit
Copy link
Member

It might be that that fat jar author didn't know about the test/spec filter settings, or that I'm completely wrong 😂

@nikunjy
Copy link
Contributor Author

nikunjy commented Sep 13, 2022

I think this is reasonable extension to the far jar launcher unless you are considering removing it. I am looking forward to using the launcher binary. Can we please merge this since it builds on the existing functionality and I don't think it conflicts with gradle filter feature.

@LeoColman LeoColman requested review from sksamuel and a team September 26, 2022 22:38
@LeoColman LeoColman enabled auto-merge (squash) September 28, 2022 18:03
@sksamuel
Copy link
Member

sksamuel commented Oct 2, 2022

I think this feature is good because in the future the launcher will need to know tests before we get into the engine, as we build out our own gradle plugin. The existing filter code works, but only when the tests are known in advance (like in the current plugin).

@sksamuel
Copy link
Member

sksamuel commented Oct 2, 2022

We can merge this once green.

@LeoColman LeoColman merged commit 740216e into kotest:master Oct 2, 2022
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.

None yet

4 participants