Hi Christoph, Such a granularity with error/failure might be also an additional requirement but still you miss the third option to JVM error which is different from test error/failure - they don;t have the same meaning.
T On Mon, Mar 14, 2022 at 10:57 AM Christoph Läubrich <m...@laeubi-soft.de> wrote: > Just wondering but maybe it would be better to configure the severity > instead of a true/false, something like: > > maven.test.failure=WARN > maven.test.error=ERROR > > would only warn about failing tests but thrw exception if starting the > test fails? > > Am 14.03.22 um 10:52 schrieb Tibor Digana: > > 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 > >>> > >> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org > For additional commands, e-mail: dev-h...@maven.apache.org > >
