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
