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

set targetTests explicitly #144

Closed
Vampire opened this issue Sep 2, 2019 · 6 comments
Closed

set targetTests explicitly #144

Vampire opened this issue Sep 2, 2019 · 6 comments
Milestone

Comments

@Vampire
Copy link
Contributor

Vampire commented Sep 2, 2019

According to http://pitest.org/quickstart/commandline/ it is recommended to always set targetTests parameter explicitly.
This plugin does only set the targetClasses explicitly which then is used for both parameters.
To save some time during targeted work I set targetClasses to the classes I'm currently working on and then wondered why there are no tests found anymore.
I'd like to recommend to also set targetTests explicitly by default.

@szpak szpak added this to the 1.4.6 milestone Sep 6, 2019
@szpak
Copy link
Owner

szpak commented Sep 9, 2019

@Vampire You may want to check 1.4.6-SNAPSHOT if it works for you.

@Vampire
Copy link
Contributor Author

Vampire commented Sep 10, 2019

I had a quick look at the commit you made.
Actually besides following the recommendation by PIT to always set it explicitly,
like you did it there will end up in the same result, the targetClasses value being used.

I don't think this makes much sense and would have led to the same confusion I had
that I described in the original comment.

Here some alternatives one of which might makes sense to you to adapt instead of what you did right now:

  • maybe it would make sense to assume some common naming convention and have as default value something like [[project.getGroup() + '.*'], ['Test', 'Spec'], ['*']].combinations()*.join()
  • or maybe list as default all classes in the test source set, suffixed with * to also catch inner classes
  • or if you don't like all of this, at least make targetTests independent from targetClasses, they can have the same default value, but if I set targetClasses to explicit classes it is still very confusing if suddenly no tests are found because they do not match that pattern

@szpak
Copy link
Owner

szpak commented Sep 11, 2019

or if you don't like all of this, at least make targetTests independent from targetClasses, they can have the same default value, but if I set targetClasses to explicit classes it is still very confusing if suddenly no tests are found because they do not match that pattern

I don't understand that. If you set targetClasses to anything it is passed to PIT:

    @Issue("https://github.com/szpak/gradle-pitest-plugin/issues/144")
    def "should set targetTest to configuration defined value"() {
        when:
            project.pitest.targetClasses = ["myClasses.*"]
            project.pitest.targetTests = ["myClasses.tests.*"]
        then:
            task.createTaskArgumentMap()['targetTests'] == "myClasses.tests.*"
    }

Unless there is (or was) a bug it should work like that also in 1.4.0.

@szpak
Copy link
Owner

szpak commented Sep 11, 2019

Regarding the general case, I'm not convinced that I want to change the default behavior. Very often people don't set anything (group is used) or set the main package in targetClasses and expect that everything will work. As I'm not able to easily (and reliably) distinguish if a developer set "main package" or "just one small subpackage" following targetClasses in targetTests would work in most of (generic) cases - it's safe. Otherwise, people setting the main package could end up with no tests (luckily detected by PIT). Focused work on mutation it's usually temporary and it is easier to explicitly set targetTests then.
Of course there are some cases where that approach will fail and people would need to set targetClasses on them own, but IMO letting it (usually) work by default is more sensible. WDYT?

@szpak
Copy link
Owner

szpak commented Sep 11, 2019

Btw, I implemented an ability to override targetTests from command line (#143), so maybe it is not so important to tune for "focused cases"?

@Vampire
Copy link
Contributor Author

Vampire commented Sep 16, 2019

Well, given you are right regarding the "people set targetClasses and expect that the tests are following suit", the current default might be good.
I was just majorly confused by that behaviour.
If you don't want to change it, fine, I'm aware of it now and especially with #143 will probably not set it in the script anyway. :-)

@szpak szpak closed this as completed in 9a92219 Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants