[SUREFIRE-1962] Unit test for ProviderInfo#isApplicable #445

slawekjaranowski · 2022-01-24T18:06:46Z

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

Tibor17 · 2022-01-25T17:38:06Z

The call new TestNgProviderInfo( null ).isApplicable() returns false.
We call new DynamicProviderInfo( null ).isApplicable() but it returns true.
The impl in DynamicProviderInfo should be a bit more similar to other impl of ProviderInfo and it should not only return the same value without the null check at least.

Tibor17 · 2022-01-25T17:53:01Z

If you read the name of method like a news autoDetectOneWellKnownProvider() it says that one should be detected.
We use to return List<ProviderInfo>. I know why it is. It simplifies the caller but it is not alright. If we have returned Optional<ProviderInfo> the method would be called easily .map(singletonList()).orElse(emptyList()) with Java 8.

slawekjaranowski · 2022-01-25T18:21:21Z

changes for DynamicProviderInfo implementation should be done in separate PR with new issue.

also changing for autoDetectOneWellKnownProvider method is out of scope for this PR.

Tibor17 · 2022-01-25T18:22:46Z

@slawekjaranowski
Your tests have discovered these cosmetic issues, so yes, pls open new tickets.

slawekjaranowski · 2022-01-26T20:12:07Z

issues created and linked

Tibor17 · 2022-01-27T08:31:45Z

...on/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoProvidersInfoTest.java

+    public void jUnitPlatformProviderApplicable()
+    {
+        Artifact junitPlatform = mock( Artifact.class );
+        ProviderInfo providerInfo = mojo.new JUnitPlatformProviderInfo( junitPlatform, aTestClassPath() );


It would be great to have unit tests for JUnitPlatformProviderInfo because the method getProviderClasspath() is not covered by unit tests, only covered by ITs. The documentation describes the behavior and the provider classpath is always resolved to necessary libraries including API and junit5 impl libraries.

it should be done is separate PR - many cases for ``getProviderClasspath`

https://issues.apache.org/jira/browse/SUREFIRE-1991

Tibor17 · 2022-01-27T08:37:44Z

...on/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoProvidersInfoTest.java

+        ProviderInfo providerInfo = mojo.new JUnit4ProviderInfo( junit, junitDep );
+
+        // ??? only existing for junitDep in any version is checked
+        assertThat( providerInfo.isApplicable() ).isTrue();


It looks like the caller's logic guarantees that the artifact coordinates are junit:junit-dep and JUnit4ProviderInfo makes only the null check. hm...

current state - it is done in this way

The point of writing the tests is that my eyes stay blind, I should not see the implementation...

code assume that input artifacts is correct .. for me looks like mixed responsibility
ProviderInfo should take as input whole classpath of project and take decision on it in one place.

Now we search artifacts in separate methods in AbstractSurefireMojo and pass only result to ProviderInfo ...
in AbstractSurefireMojo should not be any separate logic for it.

But it is another case and place to improvement.

Tibor17 · 2022-01-27T08:39:28Z

...on/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoProvidersInfoTest.java

+    {
+        Artifact junit = aArtifact( "4.5" );
+        ProviderInfo providerInfo = mojo.new JUnit4ProviderInfo( junit, null );
+        assertThat( providerInfo.isApplicable() ).isTrue();


If the version is checked, why the groupdId and artifactId is not checked? The caller is checking it?

current state ...
test for only JUnit4ProviderInfo not caller

I am talking about different things:
Why only version is checked? Why the groupId and artifactId are not checked in the impl?
Your artifact is test:test:4.5 in this test. A whatever artifact coordinates *:*:4.5 would pass this test. Do you know what I mean?

I don't know why only version is checked, probably is assumptions that input is correct ... artifact is provided by method getJunitArtifact which search specific groupId and artifactId

The purpose of writing the uni tests is hiding your eyes and you should write the tests without knowing the implementation, even before the impl exists. Here the impl exists, therefore we should hide our eyes. Then new issues would rise up.

Tibor17 · 2022-01-27T08:42:27Z

...on/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoProvidersInfoTest.java

+    {
+        Artifact junit = aArtifact( "4.7" );
+        ProviderInfo providerInfo = mojo.new JUnit4ProviderInfo( junit, null );
+        // ??? it is ok for 4.7 ???


This is ok, it is suitable provider info also for the 4.7+ if you did not configure parallel/groups.
But I see that you have missed the JUnitCoreProviderInfo - it is for 4.7+ and parallel/groups.

There is separate test for it AbstractSurefireMojoJunitCoreProvidersInfoTest

slawekjaranowski · 2022-02-24T07:26:58Z

@Tibor17 Do we want to merge it? I need rebase it.

slawekjaranowski · 2022-03-20T18:53:29Z

@Tibor17 - ping

slawekjaranowski mentioned this pull request Jan 24, 2022

[SUREFIRE-1962] Unit test for ProviderInfo#isApplicable #398

Closed

8 tasks

slawekjaranowski requested a review from Tibor17 January 24, 2022 18:08

Tibor17 reviewed Jan 27, 2022

View reviewed changes

slawekjaranowski requested a review from Tibor17 January 27, 2022 16:40

slawekjaranowski force-pushed the SUREFIRE-1962 branch from e054b90 to ca1744d Compare December 22, 2022 11:31

[SUREFIRE-1962] Unit test for ProviderInfo#isApplicable

f1c2a62

slawekjaranowski force-pushed the SUREFIRE-1962 branch from ca1744d to f1c2a62 Compare December 22, 2022 14:10

slawekjaranowski merged commit 0097a41 into master Jan 3, 2023

slawekjaranowski deleted the SUREFIRE-1962 branch January 3, 2023 23:00

slawekjaranowski added the maintenance label Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-1962] Unit test for ProviderInfo#isApplicable #445

[SUREFIRE-1962] Unit test for ProviderInfo#isApplicable #445

slawekjaranowski commented Jan 24, 2022

Tibor17 commented Jan 25, 2022

Tibor17 commented Jan 25, 2022 •

edited

Loading

slawekjaranowski commented Jan 25, 2022

Tibor17 commented Jan 25, 2022

slawekjaranowski commented Jan 26, 2022

Tibor17 Jan 27, 2022

slawekjaranowski Jan 27, 2022

slawekjaranowski Jan 27, 2022

Tibor17 Jan 27, 2022

slawekjaranowski Jan 27, 2022

Tibor17 Jan 27, 2022

slawekjaranowski Jan 27, 2022

Tibor17 Jan 27, 2022

slawekjaranowski Jan 27, 2022

Tibor17 Jan 27, 2022 •

edited

Loading

slawekjaranowski Jan 27, 2022

Tibor17 Jan 28, 2022

Tibor17 Jan 27, 2022

slawekjaranowski Jan 27, 2022

slawekjaranowski commented Feb 24, 2022

slawekjaranowski commented Mar 20, 2022

[SUREFIRE-1962] Unit test for ProviderInfo#isApplicable #445

[SUREFIRE-1962] Unit test for ProviderInfo#isApplicable #445

Conversation

slawekjaranowski commented Jan 24, 2022

Tibor17 commented Jan 25, 2022

Tibor17 commented Jan 25, 2022 • edited Loading

slawekjaranowski commented Jan 25, 2022

Tibor17 commented Jan 25, 2022

slawekjaranowski commented Jan 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slawekjaranowski commented Feb 24, 2022

slawekjaranowski commented Mar 20, 2022

Tibor17 commented Jan 25, 2022 •

edited

Loading

Tibor17 Jan 27, 2022 •

edited

Loading