Re: [IT MNG-5958]: Please review integration test for MNG-5958

Tue, 10 Jan 2017 02:52:15 -0800

I remember MNG-5805 and noticed the change of the signature. I expected this code to be Maven-internal only, so it looked fine to me. Now it seems it is not.
We had the same issue when improving toolchains, which required a signature change as well (being able to merge global with user toolchains). The assumption was that maven-toolchains-plugin was the only plugin using this specific code, so we adjusted it to make it compatible with both signatures. Again there was another thirdparty maven-plugin hitting the same issue. 


I think we should accept there are 2 signatures in the world now. On the  
plugin side there's always extra handling to be done, either refuse  
certain Maven versions or cope with both signatures.



Since the damage is already done and because the change gives you more  
info instead of less, I would advice to keep the code as-is and to provide  
a little code-example to solve this with reflection.


And yes, this still means we need to create ITs for both situations.

Robert


On Tue, 10 Jan 2017 10:48:32 +0100, Stephen Connolly  
<[email protected]> wrote:



It sounds to me that the intent for 3.3.9 was that it would work with
<phases> and the test confirmed it as working that way.


As such the impression I get is that this is neither a false positive  
nor a

false negative test. There should have been a <lifecyclePhases> test for
3.3.9, and we are intentionally changing the behaviour with <phases> so
that it no longer supports the new syntax because support for the new
syntax broke backwards compatibility.


So my vote is that we mark the existing test as range=[3.3.9,3.5.0) and  
add
a new test (which is a clone of this but switched to <lifecyclePhases>  
that

is active for the range=[3.3.9,)

WDYT?

(Requesting second opinion from Jason, Hervê & Robert)

-Stephen

On Tue 10 Jan 2017 at 08:30, Anton Tanasenko <[email protected]>
wrote:


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 <
[email protected]>

:



> 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 <

> [email protected]> 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  
<[email protected]>


> > 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 <[email protected]>:

> >>

> >>

> >>

> >> > 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 <[email protected]>

> >>

> >> > >       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 <[email protected]>

> >>

> >> > >       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 <[email protected]>

> >>

> >> > >       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 <[email protected]>

> >>

> >> > >       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 <[email protected]>

> >>

> >> > >       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 <[email protected]>

> >>

> >> > >       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.

--

Sent from my phone


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]























					Reply via email to