You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/04/01 08:17:48 UTC

[GitHub] [maven-surefire] CMoH opened a new pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

CMoH opened a new pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388


   Don't require users to add junit dependencies on all projects when using
   groups in surefire/failsafe at the reactor-level
   
   This is a proposal on how to achieve this effect, as described in the issue. Please guide me if you have expectations on how to structure this check in a better way (such as a separate method instead of a boolean?)
   
   I have prepared a demo test-case project at https://github.com/CMoH/surefire1266-sample to illustrate the situation and the fix. I can add it to the `surefire-it` folder if you find it worthwhile - maybe in a form more suitable for that purpose. The current incarnation is intended to depict the situation I am faced with, the one that motivated this contribution.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] CMoH edited a comment on pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
CMoH edited a comment on pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#issuecomment-1019363784


   @Tibor17 Here is my experience bumping into this problem:
   
   1. We want to add test groups to allow running only the integration tests against a particular external service (depending on availability). After creating categories and making it work, the full multi-module build fails against some projects, including those that don't have any tests.
   2. I look at the plugin sources, and find that the check you mentioned above only looks if the test folder exists, so I dutifully remove empty `src/test` folders across my working directory. It appears the IDE has created them and well, they just stayed there.
   3. The problem persists for some of our modules, where we have hamcrest matchers in the `src/test` directory for model classes in that jar, but no tests - since there are just beans inthere anyway. We are accustomed to import the test jar in higher-level modules, thus keeping the matchers in-line with the models. This also applied to the newly introduced high-level jar that containing group definitions for the entire reactor.
   
   I imagine we can work around this limitation by extracting these out of the test artifact into a new module, having a `-test` suffix maybe, thus moving the classes from `src/test` into `src/main`, with all the management consequences (having module pairs, educating the team etc)
   
   To me, a more elegant solution was to delay the test in surefire until it can verify that the particular jar has any tests to run, which is what this contribution was about. I am confused how come others have not run into similar problems when trying to access this tagging feature of junit, because this becomes interesting later in the project, when things get complex.
   
   On the other hand, I have to reprise the branch where I was working on this idea and revisit it.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#issuecomment-1019218934


   I have reviewed our implementation again in the MOJO.
   The method `verifyParameters()` was designed to handle your use case and simply it skips all the checks if [there are no tests to run](https://github.com/apache/maven-surefire/blob/4e900b682a58a5243a1560186cea34e431e51af2/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L1125).
   Perhaps we have not asked you the most important question. Why this part of code did not catch your use case?
   Do you deliver the tests as a test dependency artifact?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#discussion_r748776137



##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -944,6 +944,7 @@ public void execute()
                         return;
                 }
             }
+            verifyParameters( true );

Review comment:
       @CMoH @slawekjaranowski 
   Here I can see that the method `verifyParameters()` is called twice. Why?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#issuecomment-968393391


   @CMoH yes sure, feel free to rebase the branch.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] CMoH commented on pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
CMoH commented on pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#issuecomment-1019363784


   @Tibor17 Here is my experience bumping into this problem:
   
   1. We want to add test groups to allow running only the integration tests against a particular external service (depending on availability). After creating categories and making it work, the full multi-module build fails against some projects, including those that don't have any tests.
   2. I look at the plugin sources, and find that the check you mentioned above only looks if the test folder exists, so I dutifully remove empty `src/test` folders across my working directory. It appears the IDE has created them and well, they just stayed there.
   3. The problem persists for some of our modules, where we have hamcrest matchers in the `src/test` directory for model classes in that jar, but no tests - since there are just beans inthere anyway. We are accustomed to import the test jar in higher-level modules, thus keeping the matchers in-line with the models. This also applied to the newly introduced high-level jar that containing group definitions for the entire reactor.
   
   I imagine we can work around this limitation by extracting these out of the test artifact into a new module, having a `-test` suffix maybe, thus moving the classes from `src/test` into `src/main`, with all the management consequences (having module pairs, educating the team etc)
   
   To me, a more elegant solution was to delay the test in surefire until it can verify that the particular jar has any tests to run, which is what this contribution was about. I am confused how come others have not run into similar problems when trying to access this tagging feature of junit, because this becomes interesting later in the project, when things get complex.
   
   On the other hand, I have taken the wrong decision to not apply these junit groups to my code base (using my own surefire fork until this gets through), so I will have to reprise the branch and revisit what I wanted to achieve with all this :)
   
    


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] slawekjaranowski commented on pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#issuecomment-968119001


   I put some comments on jira issue.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] CMoH closed pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
CMoH closed pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] CMoH commented on a change in pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
CMoH commented on a change in pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#discussion_r748769750



##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -1101,7 +1102,7 @@ else if ( out.isDirectory() )
         }
     }
 
-    boolean verifyParameters()
+    boolean verifyParameters( boolean pluginActive )

Review comment:
       I agree the name choice is not be the best. Please note that I created this to prototype a way to solve it, as well as to point out this design issue visible in `verifyParameters()` (which does an insufficient, but speedy check, if the `test` folder exists in a module). The second, more expensive check, is done in `scanForTestClasses()`.
   
   The reasoning behind it is that some fast parameter validation can be done based purely on configuration, while a better validation can be done after inspecting the test classes, which enables more precise heuristics.
   
   This pointed me to the thought that there is an opportunity to implement these two phases: some parameters need to be checked before looking at tests, to determine if it's worth looking for tests. Next some parameters are not worth checking unless certain test providers and annotations are actually in use (the second phase).
   
   I think your decision to make is whether such a design is desirable or not. I can adjust the contribution for a better design, if desired.
   
   However, I noticed in the issue you want to solve it in a different way. Please close this merge request if you don't need it anymore.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#issuecomment-968354783


   @slawekjaranowski 
   I am investigating the Jenkins build. After we have got a new Jenkins build system the build appeared unstable.
   Let's see what we would find in:
   https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-surefire/job/jenkins/


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] CMoH commented on pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
CMoH commented on pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#issuecomment-968392091


   > @slawekjaranowski I am investigating the Jenkins build. After we have got a new Jenkins build system the build appeared unstable. Let's see what we would find in: https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-surefire/job/jenkins/
   
   I see something related to the PR template referring the jenkins build on master. Worth a rebase?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] CMoH commented on a change in pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
CMoH commented on a change in pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#discussion_r748779941



##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -944,6 +944,7 @@ public void execute()
                         return;
                 }
             }
+            verifyParameters( true );

Review comment:
       The short answer is: 
   - the first call is meant to run the fast parameters check, also determining if the plugin is enabled
   - the second is meant to run more checks, those that are only applicable to modules that do have test classes. 
   
   I believe such a split is necessary, since the test `getTestClassesDirectory().exists()` at the beginning of `verifyParameters()` is insufficient. The test directory might exist, empty, or containing files/classes which don't provide tests to run.
   
   The actual problem for me here is `warnIfDefunctGroupsCombinations()`, which should not crash for modules that don't have tests due to parameters inherited from a parent pom.
   
   
   The correct, more resource-expensive check, is done by `scanForTestClasses()`, and the second call to `verifyParameters()` only has any effect if there are indeed tests to run (and `failIfNoTests==false`).
   
   
   The second benefit is that the splitting the responsibilities of `verifyParameters()` into two methods (although I only started with a quick fix via a boolean parameter) would help as placeholders to continue refining the validation code.
   
   I touched on this also in [this comment](https://github.com/apache/maven-surefire/pull/388#discussion_r748769750) below.
   
   I am able to assist with changes, if you feel this path is helpful to the plugin.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #388: [SUREFIRE-1266] Skip junit dependency check if module has no tests to run

Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #388:
URL: https://github.com/apache/maven-surefire/pull/388#discussion_r748754593



##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -944,6 +944,7 @@ public void execute()
                         return;
                 }
             }
+            verifyParameters( true );

Review comment:
       second call? in one methd.

##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -1101,7 +1102,7 @@ else if ( out.isDirectory() )
         }
     }
 
-    boolean verifyParameters()
+    boolean verifyParameters( boolean pluginActive )

Review comment:
       What does `pluginActive` means ... ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org