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

Refer to correct property in skipping-tests doc #262

Merged
merged 1 commit into from Dec 24, 2020

Conversation

qerub
Copy link

@qerub qerub commented Jan 15, 2020

I found a little error when reading https://maven.apache.org/surefire/maven-failsafe-plugin/examples/skipping-tests.html :

You can also skip the tests via the command line by executing the following command:

mvn install -DskipITs

Since skipTests is also supported by the Surefire Plugin, this will have the effect of not running any tests. If, instead, you want to skip only the integration tests being run by the Failsafe Plugin, you would use the skipITs property instead:

mvn install -DskipITs

The former command should refer to skipTests instead.

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 17, 2020

This is a misunderstanding because skipTests is meant to be related to the unit tests and thus the maven-surefire-plugin. We know that skipTests is also utilized by maven-failsafe-plugin but that's wrong design which was confirmed by the emeritus developers of surefire. The problem with this design is the fact that you are not able to skip unit tests and ITs totally separatelly. So it is worth not to present this detail in the documentation and it is worth to recommend using skipITs for maven-failsafe-plugin and consider the skipTests in maven-failsafe-plugin as a design issue. We have a JIRA ticket for this incident.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qerub
Copy link
Author

qerub commented Jan 23, 2020

I think we're mixing up three different issues here:

  1. what the author of the current documentation had in mind when they wrote it.
  2. the general state of the current documentation (that IMHO should describe current behavior even if it's bad).
  3. the current behavior of the plugin (w.r.t. skipTests).

This mini-PR only addresses point 1. Working on 2 doesn't seem valuable until 3 has been resolved.

@Tibor17
Copy link
Contributor

Tibor17 commented Jan 28, 2020

The author of the doc separated the text been dedicated to the Surefire and Failsafe plugin using IF-ELSE decision making in one doc file. That's the (1). We do not copy the doc from Surefire module to the Failsafe module and we only switch few lines of text which is maybe 10%.

The consequence of this PR is (2) and that means the we will have a big problem to split skipping Surefire tests and Failsafe test in the future. This is the impact I can see with this PR. The goal should be to have two properties.

Since the Maven knows only Surefire bound to the phase test by default and the Failsafe is not bound to any phase, it's the property skipTests which should stick only to Surefire and unit tests. The Failsafe has skipITs property and the bug is with skipTests been used in Failsafe. Removing skipTests from the logic in Failsafe would isolate both plugins and the impact in the community will be small because we have cca 25% of Failsafe users and Surefire has 75% users. Some users asked me for providing a common property skipping both plugins but we do not have to introduce it because the property exists maven.test.skip.exec.

@qerub
Copy link
Author

qerub commented Feb 2, 2020

The author of the doc separated the text been dedicated to the Surefire and Failsafe plugin using IF-ELSE decision making in one doc file. That's the (1). We do not copy the doc from Surefire module to the Failsafe module and we only switch few lines of text which is maybe 10%.

The docs are separated using IF-ELSE but the author of the current documentation clearly wanted to document both skipITs (firstly) and skipTests. This is the section in its entirety:

  To skip running the tests for a particular project, set the <<skipITs>> property to <<true>>.
 
 +---+
 […long example with <skipITs>true</skipITs>…]
 +---+
 
  You can also skip the tests via the command line by executing the following command:
 
 +---+
-mvn install -DskipITs
+mvn install -DskipTests
 +---+
 
  Since <<<skipTests>>> is also supported by the ${thatPlugin} Plugin, this will have the effect
  of not running any tests.  If, instead, you want to skip only the integration tests
  being run by the ${thisPlugin} Plugin, you would use the <<<skipITs>>> property instead:
 
 +---+
 mvn install -DskipITs
 +---+

Please either merge or close this PR instead of leaving it hanging.

@eolivelli
Copy link
Contributor

I believe that this correction is valid. Merging.
thank you @qerub

@eolivelli eolivelli merged commit be236f7 into apache:master Dec 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants