Hi Slawomir,
Your second second example -DargLine=-Xxx is used in an integration test
in order to force producing exceptions, but IMHO it wants to simulate two
different situations.
In such cases the JVM:
1. fails on JVM startup because of low RAM
(fails the JVM on --add-reads or illegal VM parameter -Xxx and the user
wants to fail the build due to he considers this as a config error)
2. fails on OOM during the test - another CI process concurrently consumes
RAM too and randomly some processes reach OOM
(OOM during the tests - should not fail the build with
-Dmaven.test.failure.ignore=true)
We are not able to distinguish between JVM and ForkedBooter startup errors
due to the ForkedBooter does not notify the plugin with an event "I am
alive".
Currently this config parameter has this feature description in Javadoc:
*Set this to "true" to ignore a failure during testing. Its use is NOT
RECOMMENDED, but quite convenient on occasion.*
So, we have two options to fix it.
1. implement one more config param, or make a string from boolean in the
current one, or
2. distinguish between JVM and ForkedBooter startup by implementing a new
event HELLO, and use SurefireBooterForkException with some
marker forkStarted:boolean.
exit(1) is our custom error code, so we know that the ForkedBooter has
started and the build should not fail because you have
used -Dmaven.test.failure.ignore=true.
Personally, I would vote for impl #2.
Any thoughts?
Tibor
On Mon, Mar 14, 2022 at 6:18 PM Slawomir Jaranowski <s.jaranow...@gmail.com>
wrote:
> pon., 14 mar 2022 o 10:52 Tibor Digana <tibordig...@apache.org>
> 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 <
> 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
> > > >
> > >
> >
>
>
> --
> Sławomir Jaranowski
>
