maven-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anton Tanasenko <atg.sleepl...@gmail.com>
Subject Re: [IT MNG-5958]: Please review integration test for MNG-5958
Date Tue, 10 Jan 2017 08:30:24 GMT
3.3.9 (in 5805) introduced an additional syntax for specifying lifecycle
goals as
'<lifecyclePhases><..><mojos><mojo><goal/><configuration/><dependencies/></mojo</mojos</...></lifecyclePhases>'
in addition to '<phases><...>[goals as text]</...></phases>', but
due to
implementation, it was also supported using the 'phases' parent node and
the test was using that one as well.
This broke binary compatibility, which is fixed in 5958, but as a
side-effect, 'phases' node can no longer be used for the new syntax.

If strictly following the rules that you brought up, the test for 5805
should really be duplicated and changed to use the new syntax starting
3.5.0, but to be honest, I'm not sure if it's really worth it.

For one, this feature was not announced in any meaningful way, I used it
for a POC at my old job (in fact using 'lifecyclePhases'), which will never
be used, and I've also suggested it in a different issue (also with the
'lifecyclePhases') to a user.
So for the sake of the test representing the feature, if anyone comes
across it while looking for a way to use it, I'd also leave only one
instance of the test to prevent the confusion.

And for two, there are multiple dirs that are used by the test under
core-it-support and core-it-suite, most of which would need to be
duplicated. For a feature so seldomly used, duplicating would do more harm
than good.


Given the above, I'd say the test was in error in the first place, it
should have been written with 'lifecyclePhases' from start.



2017-01-10 9:51 GMT+02:00 Stephen Connolly <stephen.alan.connolly@gmail.com>
:

> I'll rephrase.
>
> That test is currently passing on 3.3.9. Why?
>
> If that testing passing on 3.3.9 because 3.3.9 was (badly) designed to work
> that way? If yes then the test stays, change the range to [3.3.9,3.5.0) and
> duplicate the test with the duplicate having the change and the range being
> [3.5.0,)
>
> If that test is passing on 3.3.9 because the test doesn't actually test
> what it was intended to test, then it is fine to change that test
>
> That the new version of the test will pass on 3.3.9 doesn't because of some
> side effect is not a valid reason to change an existing test.
>
> The test was run against 3.3.9 and 3.3.9 was released with that test
> passing, unless you are absolutely certain that the test is a
> false-positive or false-negative, we do not change the test (other than
> range activation) rather we duplicate the test (second one being _mk2 or
> something like that) and ensure that the duplicate has a different range
> activation
>
> HTH
>
> On 10 January 2017 at 07:32, Stephen Connolly <
> stephen.alan.connolly@gmail.com> wrote:
>
> > So 5805 should be marked as only for [3.3.9] and then copy it for the
> > rephrased version
> >
> > On Tue 10 Jan 2017 at 06:17, Anton Tanasenko <atg.sleepless@gmail.com>
> > wrote:
> >
> >> Looks about right.
> >>
> >>
> >>
> >> Stephen, the change to MNG-5805 test as part of MNG-5958 was
> intentional,
> >>
> >> since I broke binary compat in the initial implementation of the
> feature.
> >>
> >> The changed test should also work with 3.3.9 which supported both
> 'phases'
> >>
> >> and 'lifecyclePhases' for the extended config, while after MNG-5958 one
> >>
> >> should only use 'lifecyclePhases' for that.
> >>
> >>
> >>
> >> 2017-01-10 6:13 GMT+02:00 Christian Schulte <cs@schulte.it>:
> >>
> >>
> >>
> >> > Hi,
> >>
> >> >
> >>
> >> > forgot to add those email addresses in the CC. Sending it again with
> the
> >>
> >> > authors in the CC.
> >>
> >> >
> >>
> >> >
> >>
> >> > Am 01/10/17 um 00:59 schrieb Christian Schulte:
> >>
> >> > > Am 01/10/17 um 00:40 schrieb Stephen Connolly:
> >>
> >> > >> It seems you are modifying an existing test:
> >>
> >> > >> https://github.com/apache/maven-integration-testing/blob/
> >>
> >> > 8852538208e508fdc7b58d6332ca683bfc0c9373/core-it-support/
> >>
> >> > core-it-plugins/mng5805-extension/src/main/resources/
> >>
> >> > META-INF/plexus/components.xml
> >>
> >> > >>
> >>
> >> > >> Integration tests should be append-only (with rare exceptions)
> >>
> >> > >>
> >>
> >> > >> If the resource needs to be changed then mark the test with an
> upper
> >>
> >> > range
> >>
> >> > >> limit of ,3.5.0) and create the new test with a range limit of
> >> [3.5.0,
> >>
> >> > >>
> >>
> >> > >> Changing existing tests is bad and was one of the reasons why
we
> had
> >> to
> >>
> >> > >> reset
> >>
> >> > >>
> >>
> >> > >> -1 on the current formulation of this change.
> >>
> >> > >
> >>
> >> > > I am only the committer. I'll try to bring the authors of the
> commits
> >>
> >> > > into the discussion.
> >>
> >> > >
> >>
> >> > > Issue: <https://issues.apache.org/jira/browse/MNG-5958>.
> >>
> >> > >
> >>
> >> > > Commit in the core:
> >>
> >> > > <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=
> >>
> >> > 71ada08978c78d3b1416f0cf4f63942dddb171d9>
> >>
> >> > >
> >>
> >> > > author        Stuart McCulloch <mcculls@gmail.com>
> >>
> >> > >       Wed, 6 Jan 2016 12:23:06 +0100 (11:23 +0000)
> >>
> >> > >
> >>
> >> > > Commit to the ITs:
> >>
> >> > > <https://git-wip-us.apache.org/repos/asf?p=maven-
> >>
> >> > integration-testing.git;a=commit;h=8852538208e508fdc7b58d6332ca68
> >>
> >> > 3bfc0c9373>
> >>
> >> > >
> >>
> >> > > author        Anton Tanasenko <atg.sleepless@gmail.com>
> >>
> >> > >       Thu, 7 Jan 2016 03:01:28 +0100 (04:01 +0200)
> >>
> >> > >
> >>
> >> > > Issue introducing the IT which needs to be changed:
> >>
> >> > > <https://issues.apache.org/jira/browse/MNG-5805>
> >>
> >> > >
> >>
> >> > > Commits to the core:
> >>
> >> > > <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=
> >>
> >> > 3677220f6e499e97f2b47b0593bc394b689d14d6>
> >>
> >> > >
> >>
> >> > > author        Anton Tanasenko <atg.sleepless@gmail.com>
> >>
> >> > >       Sun, 19 Jul 2015 22:01:50 +0100 (00:01 +0300)
> >>
> >> > >
> >>
> >> > > <https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=
> >>
> >> > 9f7971dadbec8882b4c119345494b620d3a1f897>
> >>
> >> > >
> >>
> >> > > author        Anton Tanasenko <atg.sleepless@gmail.com>
> >>
> >> > >       Sat, 1 Aug 2015 15:02:52 +0100 (17:02 +0300)
> >>
> >> > >
> >>
> >> > > Commits to the ITs due to this:
> >>
> >> > > <https://git-wip-us.apache.org/repos/asf?p=maven-
> >>
> >> > integration-testing.git;a=commit;h=63656ffd5cd9c5287715336a5f91ba
> >>
> >> > f27c8360f1>
> >>
> >> > >
> >>
> >> > > author        Anton Tanasenko <atg.sleepless@gmail.com>
> >>
> >> > >       Sun, 19 Apr 2015 22:50:37 +0100 (00:50 +0300)
> >>
> >> > >
> >>
> >> > > <https://git-wip-us.apache.org/repos/asf?p=maven-
> >>
> >> > integration-testing.git;a=commit;h=ef7b0d378305ab62aa7b3824b80858
> >>
> >> > 8e5cb1a11e>
> >>
> >> > >
> >>
> >> > > author        Anton Tanasenko <atg.sleepless@gmail.com>
> >>
> >> > >       Mon, 27 Apr 2015 22:58:32 +0100 (00:58 +0300)
> >>
> >> > >
> >>
> >> > > The author of the IT being changed appears to be the same author who
> >>
> >> > > contributed the initial version.
> >>
> >> > >
> >>
> >> > > +1 from me (committer) for the change.
> >>
> >> > >
> >>
> >> > > Regards,
> >>
> >> > >
> >>
> >> >
> >>
> >> >
> >>
> >>
> >>
> >>
> >>
> >> --
> >>
> >> Regards,
> >>
> >> Anton.
> >>
> >> --
> > Sent from my phone
> >
>



-- 
Regards,
Anton.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message