Reverse the reversed error message #6290

[MichaelWMerritt]

Reverse the reversed error message #6290

Shows list of files not found first before listing the directories searched.
At the same time I referenced the constant for RESOURCES_FRONTEND_DEFAULT
instead of hard-coding that path.

Added two new test cases to cover single- and multi-file exception messages.

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

michaelwmerritt seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[caalador]

Reviewed 1 of 2 files at r1.
Reviewable status: 9 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @MichaelWMerritt)

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 57 at r1 (raw file):

import static com.vaadin.flow.server.frontend.FrontendUtils.NODE_MODULES;
import static com.vaadin.flow.server.frontend.FrontendUtils.WEBPACK_PREFIX_ALIAS;
import static org.junit.Assert.*;

We don't use import all from package. If you could revert this import change.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 169 at r1 (raw file):

atLeastOneRemoved

Should be fileRemoved as we only remove one file.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 170 at r1 (raw file):

        Mockito.when(logger.isInfoEnabled()).thenReturn(true);
        boolean atLeastOneRemoved = false;
        for (String imprt : getExpectedImports()) {

Do we need to loop the imports to remove a known file?

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 172 at r1 (raw file):

                Assert.assertTrue(resolveImportFile(frontendDirectory,
                        frontendDirectory, imprt).delete());

Could have a message on why it failed as else it will just read as expected true got false which makes it hard to see why the test failed also a message will make figuring the test out faster than without.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 177 at r1 (raw file):

            }
        }
        Assert.assertTrue(atLeastOneRemoved);

Should also have a failure message informing what we expected to be true.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 185 at r1 (raw file):

            illegalStateException = e;
        }
        assertNotNull(illegalStateException);

this assert should be inside the try as Assert.fail("Execute should have failed with missing files");

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 187 at r1 (raw file):

        assertNotNull(illegalStateException);

        assertThat(illegalStateException.getCause().getMessage(),

this assert could be in the catch block.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 196 at r1 (raw file):

        Mockito.when(logger.isInfoEnabled()).thenReturn(true);
        boolean atLeastOneRemoved = false;
        for (String imprt : getExpectedImports()) {

Do we need to loop the imports to remove two known files?
If they don't exist we will fail the assert, else we will succeed and a file will have been removed.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 205 at r1 (raw file):

        Assert.assertTrue(atLeastOneRemoved);

        IllegalStateException illegalStateException = null;

same as above, assertThat inside the catch block and a Assert.fail after updater.execute() instead of saving the exception and checking it twice.

[caalador]

Reviewed 1 of 2 files at r1.
Reviewable status: 9 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @MichaelWMerritt)

[MichaelWMerritt]

Reviewable status: 9 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 57 at r1 (raw file):

Previously, caalador wrote…

We don't use import all from package. If you could revert this import change.

Done.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 169 at r1 (raw file):

Previously, caalador wrote…

atLeastOneRemoved

Should be fileRemoved as we only remove one file.

I removed this boolean altogether as it is no longer needed with your other suggestion of not needing to iterate through the list of files in order to delete the known filename.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 170 at r1 (raw file):

Previously, caalador wrote…

Do we need to loop the imports to remove a known file?

Removed loop and am instead directly deleting the file based on name.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 172 at r1 (raw file):

Previously, caalador wrote…

                Assert.assertTrue(resolveImportFile(frontendDirectory,
                        frontendDirectory, imprt).delete());

Could have a message on why it failed as else it will just read as expected true got false which makes it hard to see why the test failed also a message will make figuring the test out faster than without.

Done.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 177 at r1 (raw file):

Previously, caalador wrote…

Should also have a failure message informing what we expected to be true.

Done.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 185 at r1 (raw file):

Previously, caalador wrote…

this assert should be inside the try as Assert.fail("Execute should have failed with missing files");

Done.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 187 at r1 (raw file):

Previously, caalador wrote…

this assert could be in the catch block.

Done.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 196 at r1 (raw file):

Previously, caalador wrote…

Do we need to loop the imports to remove two known files?
If they don't exist we will fail the assert, else we will succeed and a file will have been removed.

Done.

---

flow-server/src/test/java/com/vaadin/flow/server/frontend/NodeUpdateImportsTest.java, line 205 at r1 (raw file):

Previously, caalador wrote…

same as above, assertThat inside the catch block and a Assert.fail after updater.execute() instead of saving the exception and checking it twice.

Done.

[caalador]

Thank you for the fix.

Reviewed 1 of 1 files at r2.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained