pon., 14 mar 2022 o 10:52 Tibor Digana <[email protected]> napisał(a):
> Romain, it is not a bug. > Don't consider this as a bug. It was a feature request for change by > Olivier, and not a bug. > I closed both issues years ago but not because of ignorance but because the > appearance of the exceptional behavior is a wrong compromise and which does > not help anyone and even it makes the situation worse because typically > other group of users would fire a new Jira tickets. You would not be able > to satisfy two contradictory parties which have completely different > opinions, and so we use to introduce new params and this way we satisfy > both parties, they may combine the params as they wish, and everybody would > be happy and nobody would claim. Many technical solutions might exist, e.g. > param=boolean|string or new param=boolean, whatever. > > The truth is that this problem with (java --add-reads ...) happened in our > ASF environment on Java 8 which in good configuration would not happen and > should not. > It is not right way that we abuse "maven.test.failure.ignore" which would > tell us "Hey, you have illegal configuration in your build system and it > would not work by JDK design", it is not the goal of the plugin to tell you > that you have configured the build wrong because it won't and the same > configuration of the plugin (not the build) says "ignore the error". > So what is the difference: mvn test -Dmaven.test.failure.ignore=true -Dsurefire.rerunFailingTestsCount=notanumber mvn test -Dmaven.test.failure.ignore=true -DargLine=-Xxx for both I have illegal configuration in my build system, but one brak Maven build and one does not ... Yesterday I discussed this problem with Herve and we independently observed > equal opinions and that's not everything because we understood that the > purpose of the config parameter is to not throw MOJO exception which is > right, but the exceptional behavior should have an exact new param for the > exact use case. > SPI for this parameter is too much because no user would implement it for a > trivial parameter;; the SPI is used to be implemented by frameworks or > systems or application servers but this is not our case. > > > > > On Mon, Mar 14, 2022 at 9:11 AM Romain Manni-Bucau <[email protected]> > wrote: > > > +1 > > if it is to investigate a CI issue, it is generally easy to add debug > > insights (by code or agent) so a SPI sounds like the sanest for the > plugin > > to me. > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <https://rmannibucau.metawerx.net/> | Old Blog > > <http://rmannibucau.wordpress.com> | Github < > > https://github.com/rmannibucau> | > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > > < > > > https://www.packtpub.com/application-development/java-ee-8-high-performance > > > > > > > > > Le lun. 14 mars 2022 à 09:08, Guillaume Nodet <[email protected]> a > écrit > > : > > > > > If that's not currently possible, maybe a SPI should be provided so > that > > > people can use plug in extensions to analyze the test result and > override > > > it if necessary (transforming an error into a warning, storing results > > in a > > > way which is easier to use by other tools later...) ? > > > > > > Guillaume > > > > > > Le lun. 14 mars 2022 à 07:43, Christoph Läubrich <[email protected]> > a > > > écrit : > > > > > > > I also agree that the test at least should run, we use this property > to > > > > run the test and produce result and later on have a buildstep that > > > > analyze the results (and probably fail the build job). > > > > > > > > As it is not recommend, I wonder what is the recommended way to > archive > > > > something similar? > > > > > > > > Am 14.03.22 um 06:29 schrieb Olivier Lamy: > > > > > On Mon, 14 Mar 2022 at 11:55, Tibor Digana <[email protected] > > > > > > wrote: > > > > > > > > > >> In case of the user property *maven.test.failure.ignore* the MOJO > > must > > > > not > > > > >> throw any exception which is interpreted by the Maven Core as > BUILD > > > > >> SUCCESS. > > > > >> > > > > > > > > > > This is a very simple reduction of the problem description. > > > > > The documentation here > > > > > > > > > > > > > > > https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#testFailureIgnore > > > > > says > > > > > "Set this to "true" to ignore a failure during testing. Its use is > > NOT > > > > > RECOMMENDED, but quite convenient on occasion" > > > > > > > > > > Personally, I understand this to ignore failures in junit results > BUT > > > at > > > > > least I want the tests to run. > > > > > I guess this is how our users use this feature (feature we do not > > > > recommend > > > > > by the way...) > > > > > And this is perfectly explained here > > > > > > > > > > > > > > > https://issues.apache.org/jira/browse/SUREFIRE-1426?focusedCommentId=16188077&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16188077 > > > > > there is a difference between ignoring some junit failures and > > > ignoring a > > > > > configuration error because some jvm args has been misconfigured > for > > > many > > > > > reasons and surefire cannot start. > > > > > > > > > > If I follow your reasoning ("MOJO must not throw any exception ") > we > > > > should > > > > > ignore every surefire configuration error and keep running the > build > > > > until > > > > > the end and says BUILD SUCCESS > > > > > what about > > > > > > > > > > mvn test -Dsurefire.rerunFailingTestsCount=notanumber > > > > > -Dmaven.test.failure.ignore=true > > > > > > > > > > we should not throw any exceptions as you said below and keep > saying > > > > BUILD > > > > > SUCCESS? > > > > > argLine is a configuration element as any others so it should fail > > > > directly > > > > > and not be ignored especially when the surefire plugin cannot run. > > > > > > > > > > > > > > >> We have received an internal requirement to change the behavior of > > the > > > > user > > > > >> property *maven.test.failure.ignore* so that the behavior will > have > > > one > > > > >> exception. > > > > > > > > > > Suppose that you have JDK 1.8 but you use: > > > > >> /jre/java --add-reads ... > > > > >> The outcome is JVM exit with an error message. > > > > >> I agree with Herve who also says that *maven.test.failure.ignore* > > > > should > > > > >> not allow the MOJO to throw the exception. It is not a typical JVM > > > > >> segmentation fault or another JVM crash where we cannot do > anything > > > > about > > > > >> it, and where the entire build would crash in the command line > which > > > > >> of course means that the build cannot normally be interpreted as > > BUILD > > > > >> SUCCESS. So we are still on the same level of failures related to > > the > > > > test > > > > >> purposes. > > > > >> > > > > >> On the other hand, Olivier has reopened the issues SUREFIRE-1426 > and > > > > >> SUREFIRE-1681 where the exceptional behavior of the feature is > > > expected. > > > > >> This is exactly the reason why I closed these tickets several > years > > > ago > > > > and > > > > >> my proposal was to not to have any exceptions in the feature > > behavior > > > > and > > > > >> the proposal was to introduce a new user property for exact use > > cases. > > > > >> The next idea, which comes from two developers, would provide us > > with > > > > the > > > > >> same non exceptional and exact behavior of the user property > > > > >> *maven.test.failure.ignore* but it would also provide us with new > > user > > > > >> property in the case with fine grade control of the build errors, > > e.g. > > > > >> *maven.test.jvm.error.ignore*. > > > > >> > > > > > > > > > > with a default of? > > > > > honestly I just see this new parameter as introducing more > complexity > > > in > > > > an > > > > > already very complex code and not worth it. > > > > > again read both issue's comments and my comments. > > > > > Please try to have a user POV and think about making our users' > > > > experience > > > > > more simple. > > > > > > > > > > This should be very simple: > > > > > If surefire forked jvm cannot start it's build error and cannot run > > any > > > > > tests, it's a problem users want to know immediately because it can > > be > > > > for > > > > > a lot of reasons: wrong argLine, not enough memory on the system > > etc... > > > > > > > > > > AND AGAIN it is very different from ignoring junit result failures. > > > > > > > > > > Try to look at a user point of view and think about what is the use > > > case > > > > of > > > > > the property maven.test.failure.ignore=true, I guess 99% of the > time, > > > > users > > > > > wants to run all their tests (even on a CI with different matrix) > so > > > they > > > > > can collect all the tests results which has runned to see if there > is > > > any > > > > > issue for some combination of their matrix and eventually turn the > > > result > > > > > as unstable (this is a very typical use case in Jenkins and was > even > > a > > > > > built in feature of the previous Jenkins Maven plugin) > > > > > BUT if for any reasons one of the module do not start his tests > > because > > > > the > > > > > jvm cannot be start the users will not see that and will be totally > > > blind > > > > > until maybe someone look inside a very very large log file (which > > means > > > > > probably never) > > > > > > > > > > Long story short as my experience as a user facing this > problem/bug: > > > > > I had the case on a very large multi modules (~250 modules) build > of > > a > > > > very > > > > > used open source project. > > > > > I was using this maven.test.failure.ignore property but one of the > > > > modules > > > > > had a bad jpms configuration for a jdk17 profile on the CI. > > > > > The build has a matrix of 2 os and 4 jdks and different maven run > > which > > > > > means around ~60k tests and a Jenkins log file about 40M > > > > > So because of this property the build was not failing and kept > saying > > > > BUILD > > > > > SUCCESS for weeks/months and basically not testing one module with > > jdk > > > > 17... > > > > > And frankly you do not dig into a log file of 32M after each run > > > > especially > > > > > when it says success... > > > > > 3 days after the first release claiming supporting jdk 17 we > > received a > > > > bug > > > > > report about a something not working with jdk17.... > > > > > and guess what? Where was this feature supposed to be tested? > > > > > > > > > > so I frankly believe we do not need a complex new property, in this > > > case > > > > > this should fail directly because this will improve user > experience. > > > > > > > > > > > > > > > > > > > > > > > > >> > > > > >> > > > > >> I would like to open the discussion on this topic. You're welcome! > > > > >> > > > > >> > > > > >> Cheers > > > > >> Tibor > > > > >> > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > To unsubscribe, e-mail: [email protected] > > > > For additional commands, e-mail: [email protected] > > > > > > > > > > > > > > -- > > > ------------------------ > > > Guillaume Nodet > > > > > > -- Sławomir Jaranowski
