You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "clebertsuconic (via GitHub)" <gi...@apache.org> on 2023/09/12 23:03:14 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request, #4615: ARTEMIS-4397 Upgrade test
clebertsuconic opened a new pull request, #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#discussion_r1324339441
##########
tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/upgradeTest/UpgradeCurrentVersion.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <br>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <br>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.smoke.upgradeTest;
+
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import javax.jms.TextMessage;
+import java.io.File;
+
+import org.apache.activemq.artemis.tests.smoke.common.SmokeTestBase;
+import org.apache.activemq.artemis.tests.util.CFUtil;
+import org.apache.activemq.artemis.util.ServerUtil;
+import org.apache.activemq.artemis.utils.RandomUtil;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+/** This test is making sure the upgrade command would be able to upgrade a test I created with artemis 2.25.0 */
+public class UpgradeCurrentVersion extends SmokeTestBase {
+
+ File upgradedServer;
+
+ Process processServer;
+
+ @Before
+ public void beforeTest() throws Exception {
+ upgradedServer = new File(basedir + "/target/upgrade/currentVersion");
+
+ processServer = ServerUtil.startServer(upgradedServer.getAbsolutePath(), "upgradedServer", 0, 5000);
+ addProcess(processServer);
+ }
+
+ @Test
+ public void testSimpleSendReceive() throws Throwable {
Review Comment:
though the other test uses a split-config and so one would need a rework to use the same comparison config for both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1717574965
@gemmellr , @brusdev I think this is ready to be merged. We could add another test.. but if we keep doing the compare tests.
Further tests could be added later.. but I think what we have now is enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1716633386
@brusdev I need your help on this.
I don't understand the context on why you added regular expressions to the upgrade.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#discussion_r1324438010
##########
tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/upgradeTest/UpgradeCurrentVersion.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <br>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <br>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.smoke.upgradeTest;
+
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import javax.jms.TextMessage;
+import java.io.File;
+
+import org.apache.activemq.artemis.tests.smoke.common.SmokeTestBase;
+import org.apache.activemq.artemis.tests.util.CFUtil;
+import org.apache.activemq.artemis.util.ServerUtil;
+import org.apache.activemq.artemis.utils.RandomUtil;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+/** This test is making sure the upgrade command would be able to upgrade a test I created with artemis 2.25.0 */
+public class UpgradeCurrentVersion extends SmokeTestBase {
+
+ File upgradedServer;
+
+ Process processServer;
+
+ @Before
+ public void beforeTest() throws Exception {
+ upgradedServer = new File(basedir + "/target/upgrade/currentVersion");
+
+ processServer = ServerUtil.startServer(upgradedServer.getAbsolutePath(), "upgradedServer", 0, 5000);
+ addProcess(processServer);
+ }
+
+ @Test
+ public void testSimpleSendReceive() throws Throwable {
Review Comment:
@gemmellr I just want this test to validate if the server works after a self upgrade. (Which could be used to change home... etc...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4615: ARTEMIS-4397 Fixing the upgrade command
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#discussion_r1327171247
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/FileUtil.java:
##########
@@ -72,4 +82,27 @@ public static final boolean deleteDirectory(final File directory) {
return directory.delete();
}
+ public static final void copyDirectory(final File directorySource, final File directoryTarget) throws Exception {
+ Path sourcePath = directorySource.toPath();
+ Path targetPath = directoryTarget.toPath();
+
+ try {
+ Files.walkFileTree(sourcePath, new SimpleFileVisitor<>() {
+ @Override
+ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
+ Path targetDir = targetPath.resolve(sourcePath.relativize(dir));
+ Files.createDirectories(targetDir);
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
+ Files.copy(file, targetPath.resolve(sourcePath.relativize(file)), StandardCopyOption.REPLACE_EXISTING);
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
Review Comment:
Doesnt seem like this should be here, it could swallow failure to copy the dir properly?
##########
tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/upgradeTest/CompareUpgradeTest.java:
##########
@@ -137,30 +146,39 @@ private void compareDirectories(String expectedFolder, String upgradeFolder, Str
if (f.getName().endsWith(".exe")) {
Assert.assertArrayEquals(f.getName() + " is different after upgrade", Files.readAllBytes(f.toPath()), Files.readAllBytes(upgradeFile.toPath()));
} else {
- try (Stream<String> expectedStream = Files.lines(f.toPath());
- Stream<String> upgradeStream = Files.lines(upgradeFile.toPath())) {
+ compareFiles(allowExpectedWord, f, upgradeFile);
+ }
+ }
+ }
- Iterator<String> expectedIterator = expectedStream.iterator();
- Iterator<String> upgradeIterator = upgradeStream.iterator();
+ private static void compareFiles(boolean allowExpectedWord, File expectedFile, File upgradeFile) throws IOException {
+ logger.debug("comparing: {} to {}", upgradeFile, upgradeFile);
+ try (Stream<String> expectedStream = Files.lines(expectedFile.toPath());
+ Stream<String> upgradeStream = Files.lines(upgradeFile.toPath())) {
- int line = 1;
+ Iterator<String> expectedIterator = expectedStream.iterator();
+ Iterator<String> upgradeIterator = upgradeStream.iterator();
- while (expectedIterator.hasNext()) {
- Assert.assertTrue(upgradeIterator.hasNext());
+ int line = 1;
- String expectedString = expectedIterator.next().replace("Expected", "").trim();
- String upgradeString = upgradeIterator.next().trim();
- Assert.assertEquals("error on line " + line + " at " + upgradeFile, expectedString, upgradeString);
- line++;
- }
+ while (expectedIterator.hasNext()) {
+ Assert.assertTrue(upgradeIterator.hasNext());
- Assert.assertFalse(upgradeIterator.hasNext());
- }
- }
- }
+ String expectedString = expectedIterator.next().replace("Expected", "").trim();
+ String upgradeString = upgradeIterator.next().trim();
+ // there's a test in this class that will use a different name ID. on that case we replace Expected by ""
+ // on the comparison
+ if (allowExpectedWord) {
+ expectedString = expectedString.replace("Expected", "");
+ }
Review Comment:
This doesn't seem to be doing anything as the same replace is always already being done before it, when creating expectedString initially. Did you mean to remove that one?
##########
tests/smoke-tests/pom.xml:
##########
@@ -1537,6 +1537,21 @@
<staticCluster>tcp://localhost:61616</staticCluster>
</configuration>
</execution>
+ <execution>
+ <phase>test-compile</phase>
+ <id>upgrade-current-version-phrase1</id>
Review Comment:
typo, phrase1 -> phase1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1717634757
I think a system test for this is unnecessary (we have lots of tests using the default config already), far slower, and wont actually catch future unexpected changes that happen to not down the broker like this one did.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1718709174
create a server, upgrade and start the server is actually how you verify this issue.
if you want to do the hard way (compare every single file) it will take longer to develop, and you still need to keep the system test.
As a matter of fact I'm almost done with a compare test.. it took me a couple hours to do it... (probably 3 or 4) which is way more than this test takes to execute (1 second each time). and I'm probably keeping the system test as well to validate future issues during the upgrade. I think this is a valid test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] brusdev commented on pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1717374898
> @brusdev I need your help on this. I don't understand the context on why you added regular expressions to the upgrade.
@clebertsuconic I used regex mainly because the `uri` attribute of the `binding` element depends on the `-http-host` option of the `artemis create` command, i.e. the command `artemis create --http-host test ...` generates a bootstrap.xml with the following content
```
<web path="web" rootRedirectLocation="console">
<binding name="artemis" uri="http://test:8161">
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic merged pull request #4615: ARTEMIS-4397 Fixing the upgrade command
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic merged PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1719061417
> create a server, upgrade and start the server is actually how you verify this issue.
>
The code needing tested is in neither the broker or client that test uses, the broker wasnt involved at all in the bug or the fix, so I disagree thats how it should actually be tested.
The broker could still start even if a multitude of other highly similar/essentially-identical issues had occurred in the upgrade process. Using the broker+client to 'test' this by side-effect is both slow and gives a poor result. Spending 1 second (or whatever it actually is) every time the test suite is run in future while testing this type of issue, vs the comparitively much smaller amount of closely verifying the files / cli-handling, is essentially exactly how over time the test suite ends up in the state it is in now.
> if you want to do the hard way (compare every single file) it will take longer to develop, and you still need to keep the system test.
I wouldnt call it the hard way, but test development is often a time tradeoff yes. Development time now vs output quality and maintenance burden later. More time developing a good test, that runs fast, is reliable, is targeted, gives good failure messages, catches the entire general issue rather than just 1 variant that had a no-startup side effect...vs later having years of wasting time on every execution of slower poorer tests that verify by side effect, wont catch various near-identical related issues, dont give as nice a failure if they should fail, perhaps even fail sporadically....and which actually a lot of time people dont even bother to run precisely because the complete test suite is so slow and unreliable overall, which I know happens fairly regularly from the number of times I encounter it while reviewing PRs.
>
> As a matter of fact I'm almost done with a compare test.. it took me a couple hours to do it... (probably 3 or 4) which is way more than this test takes to execute (1 second each time). and I'm probably keeping the system test as well to validate future issues during the upgrade. I think this is a valid test.
It might catch _this_ issue as a complete side effect, but it wont catch other almost identical issues, which the other test will have caught already. Its just a significantly poorer test overall relative to the alternative of checking the actual CLI behaviour to verify the CLI behaviour. We already have other tests starting the broker up with the same 'default config' the upgrade test should be verifying is what is still present after such a 'non-upgrade'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4615: ARTEMIS-4397 Fixing the upgrade command
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#discussion_r1327437254
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/FileUtil.java:
##########
@@ -72,4 +82,27 @@ public static final boolean deleteDirectory(final File directory) {
return directory.delete();
}
+ public static final void copyDirectory(final File directorySource, final File directoryTarget) throws Exception {
+ Path sourcePath = directorySource.toPath();
+ Path targetPath = directoryTarget.toPath();
+
+ try {
+ Files.walkFileTree(sourcePath, new SimpleFileVisitor<>() {
+ @Override
+ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
+ Path targetDir = targetPath.resolve(sourcePath.relativize(dir));
+ Files.createDirectories(targetDir);
+ return FileVisitResult.CONTINUE;
+ }
+
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
+ Files.copy(file, targetPath.resolve(sourcePath.relativize(file)), StandardCopyOption.REPLACE_EXISTING);
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ } catch (IOException e) {
+ e.printStackTrace();
+ }
Review Comment:
Yes.... it should be taken out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#discussion_r1324249926
##########
tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/upgradeTest/UpgradeCurrentVersion.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <br>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <br>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.smoke.upgradeTest;
+
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import javax.jms.TextMessage;
+import java.io.File;
+
+import org.apache.activemq.artemis.tests.smoke.common.SmokeTestBase;
+import org.apache.activemq.artemis.tests.util.CFUtil;
+import org.apache.activemq.artemis.util.ServerUtil;
+import org.apache.activemq.artemis.utils.RandomUtil;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+/** This test is making sure the upgrade command would be able to upgrade a test I created with artemis 2.25.0 */
+public class UpgradeCurrentVersion extends SmokeTestBase {
+
+ File upgradedServer;
+
+ Process processServer;
+
+ @Before
+ public void beforeTest() throws Exception {
+ upgradedServer = new File(basedir + "/target/upgrade/currentVersion");
+
+ processServer = ServerUtil.startServer(upgradedServer.getAbsolutePath(), "upgradedServer", 0, 5000);
+ addProcess(processServer);
+ }
+
+ @Test
+ public void testSimpleSendReceive() throws Throwable {
Review Comment:
I think a likely better and faster test would be to comparable to / part-of the existing CompareUpgradeTest. We already have a current-expected-instance-config for those existing tests to compare with their old-upgraded-instance output to check for match. We can compare a new current-version-upgraded instance config with that and check they match in much the same fashion, whereas this test wouldnt notice if it did change something unless that actually breaks the server.
##########
tests/smoke-tests/src/test/java/org/apache/activemq/artemis/tests/smoke/upgradeTest/UpgradeCurrentVersion.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <br>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <br>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.activemq.artemis.tests.smoke.upgradeTest;
+
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import javax.jms.TextMessage;
+import java.io.File;
+
+import org.apache.activemq.artemis.tests.smoke.common.SmokeTestBase;
+import org.apache.activemq.artemis.tests.util.CFUtil;
+import org.apache.activemq.artemis.util.ServerUtil;
+import org.apache.activemq.artemis.utils.RandomUtil;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+/** This test is making sure the upgrade command would be able to upgrade a test I created with artemis 2.25.0 */
Review Comment:
Comment is wrong, remove? (It applied to the original test, but not this one).
##########
tests/smoke-tests/pom.xml:
##########
@@ -1318,6 +1318,31 @@
</args>
</configuration>
</execution>
+ <execution>
+ <phase>test-compile</phase>
+ <id>upgrade-current-version-phrase1</id>
+ <goals>
+ <goal>create</goal>
+ </goals>
+ <configuration>
+ <noWeb>false</noWeb>
+ <instance>${basedir}/target/upgrade/currentVersion</instance>
+ <args>
+ <arg>--disable-persistence</arg>
+ </args>
+ </configuration>
+ </execution>
+ <execution>
+ <phase>test-compile</phase>
+ <id>upgrade-current-version-phase2</id>
+ <goals>
+ <goal>upgrade</goal>
+ </goals>
+ <configuration>
+ <instance>${basedir}/target/upgrade/currentVersion</instance>
+ </configuration>
+ </execution>
+
Review Comment:
This is being inserted in the middle of existing executions used together for a test, separating them...moving this addition up (e.g line 1306) or down (e.g. line 1379) would make it more understandable later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] brusdev commented on pull request #4615: ARTEMIS-4397 Upgrade test
Posted by "brusdev (via GitHub)" <gi...@apache.org>.
brusdev commented on PR #4615:
URL: https://github.com/apache/activemq-artemis/pull/4615#issuecomment-1717378728
@clebertsuconic I have just pushed a commit to revert and fix the upgrade regex
https://github.com/apache/activemq-artemis/pull/4615/commits/76695e4b2f10f8cf1bc6cf979e6f4936974dc4b7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org