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". 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 <rmannibu...@gmail.com> 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 <gno...@apache.org> 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 <m...@laeubi-soft.de> 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 <tibordig...@apache.org> > > > 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: dev-unsubscr...@maven.apache.org > > > For additional commands, e-mail: dev-h...@maven.apache.org > > > > > > > > > > -- > > ------------------------ > > Guillaume Nodet > > >
