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 <[email protected]>
vaadin · Dec 19, 2024 · ff037b6 · ff037b6
1 parent 5b87120
commit ff037b6
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 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
@@ -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,15 +306,18 @@ 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) {
                 packageUpdater.log()
                         .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
@@ -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
@@ -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