From ff037b62a492c76a8dea0c612e69a20ccc28045a Mon Sep 17 00:00:00 2001 From: caalador Date: Thu, 19 Dec 2024 10:16:09 +0200 Subject: [PATCH] fix: do not mistakenly run 2 npm installs (#20612) * fix: do not mistakenly run 2 npm installs Fixes #19370 * format * fix test log checks. * wait a bit longer for jetty to stop * Make a marking that npm install is running. * unused import revert stopWait change --------- Co-authored-by: Teppo Kurki --- .../server/frontend/TaskRunNpmInstall.java | 35 +++++++++++++++---- .../frontend/TaskRunNpmInstallTest.java | 34 +++++++++--------- .../frontend/TaskRunPnpmInstallTest.java | 4 +-- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java index fa3b3689f77..33d1296563a 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java @@ -75,6 +75,7 @@ public class TaskRunNpmInstall implements FallibleCommand { + "%n 4) Deleting the following files from your Vaadin project's folder (if present):" + "%n node_modules, package-lock.json, webpack.generated.js, pnpm-lock.yaml, pnpmfile.js" + "%n======================================================================================================%n"; + public static final String INSTALLING = "installing"; private final NodeUpdater packageUpdater; @@ -130,16 +131,18 @@ public void execute() throws ExecutionFailedException { if (ciBuild || packageUpdater.modified || shouldRunNpmInstall()) { packageUpdater.log() - .info("Running `" + toolName + " " + command - + "` to resolve and " + .info("Running `{} {}` to resolve and " + "optionally download frontend dependencies. " - + "This may take a moment, please stand by..."); + + "This may take a moment, please stand by...", + toolName, command); + updateLocalHashForInstall(); + runNpmInstall(); updateLocalHash(); } else { packageUpdater.log() - .info("Skipping `{} {}}` because the frontend packages " + .info("Skipping `{} {}` because the frontend packages " + "are already installed in the folder '{}' and " + "the hash in the file '{}' is the same as in '{}'", toolName, command, @@ -180,6 +183,21 @@ private void updateLocalHash() { } } + private void updateLocalHashForInstall() { + try { + final JsonObject localHash = Json.createObject(); + localHash.put(HASH_KEY, INSTALLING); + + final File localHashFile = getLocalHashFile(); + FileUtils.forceMkdirParent(localHashFile); + String content = stringify(localHash, 2) + "\n"; + FileUtils.writeStringToFile(localHashFile, content, UTF_8.name()); + + } catch (IOException e) { + packageUpdater.log().warn("Failed to update node_modules hash.", e); + } + } + private File getLocalHashFile() { return new File(packageUpdater.nodeModulesFolder, INSTALL_HASH); } @@ -274,7 +292,7 @@ private boolean shouldRunNpmInstall() { // Only flow-frontend installed return true; } else { - return isVaadinHashUpdated(); + return !isVaadinHashUpdated(); } } @@ -288,7 +306,10 @@ private boolean isVaadinHashUpdated() { if (content.hasKey(HASH_KEY)) { final JsonObject packageJson = packageUpdater .getPackageJson(); - return !content.getString(HASH_KEY).equals(packageJson + if (content.get(HASH_KEY).asString().equals(INSTALLING)) { + return true; + } + return content.getString(HASH_KEY).equals(packageJson .getObject(VAADIN_DEP_KEY).getString(HASH_KEY)); } } catch (IOException e) { @@ -296,7 +317,7 @@ private boolean isVaadinHashUpdated() { .warn("Failed to load hashes forcing npm execution", e); } } - return true; + return false; } /** diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunNpmInstallTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunNpmInstallTest.java index b029202c7e3..4707b44aff1 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunNpmInstallTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunNpmInstallTest.java @@ -58,7 +58,7 @@ public class TaskRunNpmInstallTest { private ClassFinder finder; - protected Logger logger = Mockito.mock(Logger.class); + protected MockLogger logger; private File generatedFolder; @@ -68,6 +68,7 @@ public class TaskRunNpmInstallTest { @Before public void setUp() throws IOException { + logger = new MockLogger(); generatedFolder = temporaryFolder.newFolder(); npmFolder = temporaryFolder.newFolder(); File generatedPath = new File(npmFolder, "generated"); @@ -107,7 +108,7 @@ public void runNpmInstall_emptyDir_npmInstallIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test @@ -121,7 +122,7 @@ public void runNpmInstall_nodeModulesContainsStaging_npmInstallIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test @@ -165,7 +166,7 @@ public void runNpmInstall_nonEmptyDirNoLocalHash_npmInstallIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test @@ -180,12 +181,13 @@ public void runNpmInstall_nonEmptyDirNoHashMatch_npmInstallIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test public void runNpmInstall_matchingHash_npmInstallIsNotExecuted() throws IOException, ExecutionFailedException { + logger = Mockito.mock(MockLogger.class); File nodeModules = getNodeUpdater().nodeModulesFolder; nodeModules.mkdir(); @@ -202,7 +204,7 @@ public void runNpmInstall_matchingHash_npmInstallIsNotExecuted() "\\\\\\\\")), Mockito.any(), Mockito.matches(Constants.PACKAGE_JSON)); Assert.assertEquals( - "Skipping `{} {}}` because the frontend packages are already installed in the folder '{}' and the hash in the file '{}' is the same as in '{}'", + "Skipping `{} {}` because the frontend packages are already installed in the folder '{}' and the hash in the file '{}' is the same as in '{}'", captor.getValue()); } @@ -217,7 +219,7 @@ public void runNpmInstall_matchingHashButEmptyModules_npmInstallIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test @@ -232,13 +234,13 @@ public void runNpmInstallAndCi_emptyDir_npmInstallAndCiIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); deleteDirectory(nodeModules); task = createTask(true); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test @@ -276,7 +278,7 @@ public void runNpmInstall_dirContainsOnlyFlowNpmPackage_npmInstallIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test @@ -286,7 +288,7 @@ public void runNpmInstall_modified_npmInstallIsExecuted() ensurePackageJson(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test(expected = ExecutionFailedException.class) @@ -338,11 +340,11 @@ public void runNpmInstall_externalUpdateOfPackages_npmInstallIsRerun() packageJson.getObject(VAADIN_DEP_KEY).getString(HASH_KEY)); getNodeUpdater().writePackageFile(packageJson); - logger = Mockito.mock(Logger.class); + logger = new MockLogger(); task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Test @@ -411,9 +413,9 @@ protected void assertRunNpmInstallThrows_vaadinHomeNodeIsAFolder( } protected String getRunningMsg() { - return "Running `" + getToolName() + " " + getCommand() + "` to " - + "resolve and optionally download frontend dependencies. " - + "This may take a moment, please stand by..."; + return String.format( + "Running `%s %s` to resolve and optionally download frontend dependencies. This may take a moment, please stand by...", + getToolName(), getCommand()); } private String getCommand() { diff --git a/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java b/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java index 25c51397050..1b0da91867b 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java @@ -589,13 +589,13 @@ public void runPnpmInstallAndCi_emptyDir_pnpmInstallAndCiIsExecuted() getNodeUpdater().modified = false; task.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); deleteDirectory(nodeModules); TaskRunNpmInstall ciTask = createTask(true); ciTask.execute(); - Mockito.verify(logger).info(getRunningMsg()); + Assert.assertTrue(logger.getLogs().contains(getRunningMsg())); } @Override