Add constants for string literals in UI test classes #1199

anusreelakshmi934 · 2024-12-18T05:44:20Z

Fixes #1153

src/main/java/io/openliberty/tools/intellij/util/Constants.java

src/test/java/io/openliberty/tools/intellij/it/Utils/ItConstants.java

src/test/java/io/openliberty/tools/intellij/it/TestUtils.java

dessina-devasia

Change Copyright year in all files

anusreelakshmi934 · 2025-01-02T06:07:45Z

Change Copyright year in all files

Changed the Copyright year

mrglavas

@anusreelakshmi934 I see several new failures that are occurring on the Linux and Windows builds, for example:

GradleSingleModJakartaLSTest > initializationError FAILED
    java.lang.RuntimeException: Unable to open file SystemResource
        at io.openliberty.tools.intellij.it.UIBotTestUtils.openFile(UIBotTestUtils.java:700)
        at io.openliberty.tools.intellij.it.SingleModJakartaLSTestCommon.prepareEnv(SingleModJakartaLSTestCommon.java:204)
        at io.openliberty.tools.intellij.it.GradleSingleModJakartaLSTest.setup(GradleSingleModJakartaLSTest.java:40)

        Caused by:
        java.util.NoSuchElementException: List is empty.
            at kotlin.collections.CollectionsKt___CollectionsKt.first(_Collections.kt:221)
            at com.intellij.remoterobot.fixtures.Fixture.findText(Fixture.kt:46)
            at io.openliberty.tools.intellij.it.UIBotTestUtils.openFile(UIBotTestUtils.java:687)
            ... 2 more

I suspect these might be related to recent changes in computing paths in this PR. Please verify.

…artaLSTestCommon

dessina-devasia

Looks good to me. Thanks!

anusreelakshmi934 · 2025-01-14T04:28:08Z

@anusreelakshmi934 I see several new failures that are occurring on the Linux and Windows builds, for example:

GradleSingleModJakartaLSTest > initializationError FAILED
    java.lang.RuntimeException: Unable to open file SystemResource
        at io.openliberty.tools.intellij.it.UIBotTestUtils.openFile(UIBotTestUtils.java:700)
        at io.openliberty.tools.intellij.it.SingleModJakartaLSTestCommon.prepareEnv(SingleModJakartaLSTestCommon.java:204)
        at io.openliberty.tools.intellij.it.GradleSingleModJakartaLSTest.setup(GradleSingleModJakartaLSTest.java:40)

        Caused by:
        java.util.NoSuchElementException: List is empty.
            at kotlin.collections.CollectionsKt___CollectionsKt.first(_Collections.kt:221)
            at com.intellij.remoterobot.fixtures.Fixture.findText(Fixture.kt:46)
            at io.openliberty.tools.intellij.it.UIBotTestUtils.openFile(UIBotTestUtils.java:687)
            ... 2 more

I suspect these might be related to recent changes in computing paths in this PR. Please verify.

Hi @mrglavas . I have made the changes and now the test succeeds.

TrevCraw

It looks like a few spots to use the new constants may have been missed. I only marked the spots visible in the GitHub diffs. Please make sure to search through the test code to see if there are any other spots that were missed where the new constants can be used or where new constants should be created.

src/test/java/io/openliberty/tools/intellij/it/SingleModJakartaLSTestCommon.java

src/test/java/io/openliberty/tools/intellij/it/SingleModLibertyLSTestCommon.java

src/test/java/io/openliberty/tools/intellij/it/SingleModMPLSTestCommon.java

src/test/java/io/openliberty/tools/intellij/it/TestUtils.java

anusreelakshmi934 · 2025-01-15T07:11:39Z

It looks like a few spots to use the new constants may have been missed. I only marked the spots visible in the GitHub diffs. Please make sure to search through the test code to see if there are any other spots that were missed where the new constants can be used or where new constants should be created.

Hi @TrevCraw . Initially, I replaced all occurrences where the new constants could be used. However, this caused a GitHub Actions (GHA) failure where the file could not be opened. Upon investigation, I discovered that the openFile method expects the filePath parameter in the format String... filePath. Replacing it with the constant resulted in an error because the input was not provided in the required String... format.

I also tried passing the value as a String array, but this led to a compilation issue. The problem arises because the method expects multiple strings as individual arguments and does not accept an array in place of these arguments.

Expected Output is -

But if we pass the constant we will get -

If we pass the value as String array we will get -

So the file will not be opened. Hence I kept the strings in openFile as it was.

TrevCraw · 2025-01-15T23:38:55Z

If we pass the value as String array we will get -

So the file will not be opened. Hence I kept the strings in openFile as it was.

Hi @anusreelakshmi934 , looking at the image above, it looks like you used Arrays.toString() for your String array parameter. That will not work, as Arrays.toString() returns a String, not an array of Strings. You should be able to pass an array of Strings to the String... varargs parameter.

If you take a step back and look at the original code, you will noticed the pattern for both getPath() and openFile() was always set up to work with String.... We should leave that format alone and adjust the newly created constants to match the existing code patterns. It would probably be best to modify your String constants that are paths with slashes to be String arrays instead.

anusreelakshmi934 · 2025-01-16T13:26:31Z

If we pass the value as String array we will get -
So the file will not be opened. Hence I kept the strings in openFile as it was.

Hi @anusreelakshmi934 , looking at the image above, it looks like you used Arrays.toString() for your String array parameter. That will not work, as Arrays.toString() returns a String, not an array of Strings. You should be able to pass an array of Strings to the String... varargs parameter.

If you take a step back and look at the original code, you will noticed the pattern for both getPath() and openFile() was always set up to work with String.... We should leave that format alone and adjust the newly created constants to match the existing code patterns. It would probably be best to modify your String constants that are paths with slashes to be String arrays instead.

Thanks, @TrevCraw, for the clarification. Initially, when I passed the string array to the openFile method, it was being interpreted as the fifth parameter, with the project name as the fourth. This caused issues with passing the string array. To resolve this, I modified the approach by combining the projectName and the path. With this change, two of the classes are now passing, but others are still failing despite using the same approach. I am currently investigating the issue and will update the PR accordingly.

mrglavas · 2025-01-16T18:58:41Z

Thanks, @TrevCraw, for the clarification. Initially, when I passed the string array to the openFile method, it was being interpreted as the fifth parameter, with the project name as the fourth. This caused issues with passing the string array. To resolve this, I modified the approach by combining the projectName and the path. With this change, two of the classes are now passing, but others are still failing despite using the same approach. I am currently investigating the issue and will update the PR accordingly.

@anusreelakshmi934 I know you're currently working on updates to this PR so will wait for that before reviewing again. For the current state of the PR, I had similar feedback to what @TrevCraw already expressed. It should be possible to construct arrays from a combination of constants and variable values that are equivalent to the vararg syntax (for String...) that was in the original code.

mrglavas

See my recent comments. Awaiting changes before reviewing again.

TrevCraw

There are lots of places where String.join() is used. Would it be possible to pass String arrays as was done before? You can build the String arrays from your existing constants dynamically if needed. Or, in most cases, you can create new constants that combine your existing constants.

For example, you have:

CONFIG_DIR_PATH = { "src", "main", "liberty", "config" };
SERVER_XML = "server.xml";

You can create a new constant that combines the two called SERVER_XML_PATH and then use that to pass as a parameter to the necessary methods.

src/test/java/io/openliberty/tools/intellij/it/Utils/ItConstants.java

anusreelakshmi934 · 2025-01-22T07:24:42Z

Hi @TrevCraw,
I’ve addressed your comments and made the necessary changes. Please review them when you get a chance and let me know if this aligns with your expectations or if further modifications are needed. Thanks!

Additionally, I’ve removed the WLP_MSGLOG_PATH from the TestUtils file and moved it to the ItConstants file as a slash-separated path. I felt that, instead of calling Paths.get(), it might be more efficient to directly specify the path there. Could you confirm if this approach is appropriate?

TrevCraw

Hi @anusreelakshmi934 , please address the review comments below. And be sure to check the build results after making your changes.

It probably was not necessary to move MESSAGES_LOG_PATH to the ItConstants file since it is only used in one file. However, the change should be OK. Please take a look at one of the comments related to this change below.

TrevCraw · 2025-01-22T23:12:14Z

src/test/java/io/openliberty/tools/intellij/it/SingleModJakartaLSTestCommon.java

-            UIBotTestUtils.insertCodeSnippetIntoSourceFile(remoteRobot, "SystemResource.java", snippetStr, snippetChooser);
-            Path pathToSrc = Paths.get(projectsPath, projectName, "src", "main", "java", "io", "openliberty", "mp", "sample", "system", "SystemResource.java");
+            UIBotTestUtils.insertCodeSnippetIntoSourceFile(remoteRobot, ItConstants.SYSTEM_RESOURCE_JAVA, snippetStr, snippetChooser);
+            Path pathToSrc = Paths.get(projectsPath, projectName, ItConstants.SYSTEM_DIR_PATH, ItConstants.SYSTEM_RESOURCE_JAVA);


Why is SYSTEM_DIR_PATH a String and not a String[]? It would be better to keep the format the same as the original code. You can always build a new String[] constant from String[] SYSTEM_DIR_PATH and String SYSTEM_RESOURCE_JAVA.

I have updated the SYSTEM_DIR_PATH to a String[]. Additionally, I introduced new variables: SERVER_XML_PATH, SERVER_ENV_PATH, and BOOTSTRAP_PROPERTIES_PATH.

TrevCraw · 2025-01-22T23:14:36Z

src/test/java/io/openliberty/tools/intellij/it/SingleModLibertyLSTestCommon.java


        // Save the current server.xml content.
        UIBotTestUtils.copyWindowContent(remoteRobot);

        // Insert a new element in server.xml.
        try {
            UIBotTestUtils.insertStanzaInAppServerXML(remoteRobot, stanzaSnippet, 18, 40, UIBotTestUtils.InsertionType.FEATURE, true);
-            Path pathToServerXML = Paths.get(projectsPath, projectName, "src", "main", "liberty", "config", "server.xml");
+            Path pathToServerXML = Paths.get(projectsPath, projectName, ItConstants.CONFIG_DIR_PATH, ItConstants.SERVER_XML);


Same comment here as for SYSTEM_DIR_PATH above. Would be better to keep as a String[].

I have updated the SYSTEM_DIR_PATH to a String[]

TrevCraw · 2025-01-22T23:17:52Z

src/test/java/io/openliberty/tools/intellij/it/SingleModMPLSTestCommon.java

-            UIBotTestUtils.insertCodeSnippetIntoSourceFile(remoteRobot, "ServiceLiveHealthCheck.java", snippetStr, snippetChooser);
-            Path pathToSrc = Paths.get(projectsPath, projectName, "src", "main", "java", "io", "openliberty", "mp", "sample", "health","ServiceLiveHealthCheck.java");
+            UIBotTestUtils.insertCodeSnippetIntoSourceFile(remoteRobot, ItConstants.SERVICE_LIVE_HEALTH_CHECK_JAVA, snippetStr, snippetChooser);
+            Path pathToSrc = Paths.get(projectsPath, projectName, ItConstants.HEALTH_DIR_PATH, ItConstants.SERVICE_LIVE_HEALTH_CHECK_JAVA);


Same comment here as for SYSTEM_DIR_PATH and CONFIG_DIR_PATH above. Would be better to keep as a String[].

I have updated the SYSTEM_DIR_PATH to a String[]

TrevCraw · 2025-01-22T23:20:13Z

src/test/java/io/openliberty/tools/intellij/it/SingleModMPLSTestCommon.java

-            UIBotTestUtils.insertConfigIntoMPConfigPropertiesFile(remoteRobot, "microprofile-config.properties", cfgSnippet, cfgNameChooserSnippet, cfgValueSnippet, true);
-            Path pathToMpCfgProperties = Paths.get(projectsPath, projectName, "src", "main", "resources", "META-INF", "microprofile-config.properties");
+            UIBotTestUtils.insertConfigIntoMPConfigPropertiesFile(remoteRobot, ItConstants.MPG_PROPERTIES, cfgSnippet, cfgNameChooserSnippet, cfgValueSnippet, true);
+            Path pathToMpCfgProperties = Paths.get(projectsPath, projectName, String.join(File.separator, ItConstants.META_INF_DIR_PATH), ItConstants.MPG_PROPERTIES);


Why do a String.join() here? The original code is passing a String[].

I have removed the String.join() and combined the projectName and MPCFG path using combinePath method.

TrevCraw · 2025-01-22T23:20:59Z

src/test/java/io/openliberty/tools/intellij/it/SingleModMPLSTestCommon.java

@@ -294,19 +296,19 @@ public void testQuickFixInMicroProfileConfigProperties() {
        String correctedValue = "mp.health.disable-default-procedures=true";
        String expectedHoverData = "Type mismatch: boolean expected. By default, this value will be interpreted as 'false'";

-        Path pathToMpCfgProperties = Paths.get(projectsPath, projectName,"src", "main", "resources", "META-INF", "microprofile-config.properties");
+        Path pathToMpCfgProperties = Paths.get(projectsPath, projectName, String.join(File.separator, ItConstants.META_INF_DIR_PATH), ItConstants.MPG_PROPERTIES);


Same as above comment on String.join().

I have removed the String.join() and combined the projectName and MPCFG path using combinePath method

TrevCraw · 2025-01-22T23:22:40Z

src/test/java/io/openliberty/tools/intellij/it/TestUtils.java

@@ -541,7 +540,7 @@ public static boolean isServerStopNeeded(String wlpInstallPath) {
     */
    public static void checkDebugPort(String absoluteWLPPath, int debugPort) throws IOException {
        // Retrieve the WLP server.env file path
-        Path serverEnvPath = Paths.get(absoluteWLPPath, "wlp", "usr", "servers", "defaultServer", "server.env");
+        Path serverEnvPath = Paths.get(absoluteWLPPath, ItConstants.DEFAULT_SERVER_PATH, ItConstants.SERVER_ENV);


Same comment here as for SYSTEM_DIR_PATH, CONFIG_DIR_PATH and HEALTH_DIR_PATH above. Would be better to keep as a String[].

I have updated it to String[].

src/test/java/io/openliberty/tools/intellij/it/Utils/ItConstants.java

src/test/java/io/openliberty/tools/intellij/it/TestUtils.java

- MPG_PROPERTIES changed to MPCFG_PROPERTIES - concatenate the Strings for wlpMsgLogPath

turkeylurkey · 2025-01-23T22:11:27Z

src/test/java/io/openliberty/tools/intellij/it/SingleModJakartaLSTestCommon.java

@@ -12,6 +12,7 @@
 import com.automation.remarks.junit5.Video;
 import com.intellij.remoterobot.RemoteRobot;
 import com.intellij.remoterobot.fixtures.JTreeFixture;
+import io.openliberty.tools.intellij.it.Utils.ItConstants;


I'd like to suggest using import static io.openliberty.tools.intellij.it.Utils.ItConstants.* so that you do not have to specify the class name every time you use a constant in all these test files. It gets repetitive.

anusreelakshmi934 requested review from aparnamichael, staicy123 and vaisakhkannan December 18, 2024 05:44

vaisakhkannan requested changes Dec 19, 2024

View reviewed changes

src/main/java/io/openliberty/tools/intellij/util/Constants.java Outdated Show resolved Hide resolved

anusreelakshmi934 requested review from vaisakhkannan and TrevCraw December 19, 2024 04:51

vaisakhkannan requested changes Dec 19, 2024

View reviewed changes

src/test/java/io/openliberty/tools/intellij/it/Utils/ItConstants.java Show resolved Hide resolved

src/test/java/io/openliberty/tools/intellij/it/TestUtils.java Outdated Show resolved Hide resolved

anusreelakshmi934 requested a review from dessina-devasia January 1, 2025 13:55

dessina-devasia reviewed Jan 1, 2025

View reviewed changes

anusreelakshmi934 requested review from dessina-devasia, mrglavas, TrevCraw and vaisakhkannan and removed request for TrevCraw, staicy123 and aparnamichael January 2, 2025 06:08

mrglavas requested changes Jan 8, 2025

View reviewed changes

mrglavas assigned anusreelakshmi934 Jan 8, 2025

anusreelakshmi934 marked this pull request as draft January 9, 2025 05:46

anusreelakshmi934 added 10 commits January 13, 2025 14:21

changed to constants in SingleModLibertyLSTestCommon and SingleModJak…

5268e33

…artaLSTestCommon

SingleModMPLSTestCommon file

1e62f89

SingleModMPProjectTestCommon file

02c2113

SingleModNLTRestProjectTestCommon file

da9f216

modified other strings to constants

daa7096

Moved into constants file under the test directory

769d3d3

Changed copyright year

fcac98e

copyright header added

02a2265

SingleModJakartaLSTestCommon class

ce62614

SingleModLibertyLSTestCommon class

42261b6

anusreelakshmi934 added 2 commits January 13, 2025 19:55

SingleModLibertyLSTestCommon updated

4761fe1

SingleModMPLSTestCommon updated

698703d

anusreelakshmi934 marked this pull request as ready for review January 13, 2025 14:29

removed extra imports

6f5f2f5

dessina-devasia previously approved these changes Jan 13, 2025

View reviewed changes

dessina-devasia requested a review from mrglavas January 13, 2025 18:55

TrevCraw requested changes Jan 14, 2025

View reviewed changes

messages.log path

97eac05

anusreelakshmi934 dismissed dessina-devasia’s stale review via 97eac05 January 15, 2025 06:15

mrglavas requested changes Jan 16, 2025

View reviewed changes

anusreelakshmi934 added 3 commits January 17, 2025 15:52

path changed to string array

e64cdbd

Update Constants.java

64b7a81

Update SingleModMPLSTestCommon.java

f1d400d

anusreelakshmi934 requested a review from dessina-devasia January 17, 2025 10:39

dessina-devasia previously approved these changes Jan 17, 2025

View reviewed changes

anusreelakshmi934 requested review from mrglavas and TrevCraw January 20, 2025 04:47

TrevCraw requested changes Jan 21, 2025

View reviewed changes

src/test/java/io/openliberty/tools/intellij/it/Utils/ItConstants.java Show resolved Hide resolved

addressed review comments

31ca81d

anusreelakshmi934 dismissed dessina-devasia’s stale review via 31ca81d January 22, 2025 07:12

TrevCraw requested changes Jan 22, 2025

View reviewed changes

anusreelakshmi934 added 2 commits January 23, 2025 12:04

Updates

3f1b7b3

- MPG_PROPERTIES changed to MPCFG_PROPERTIES - concatenate the Strings for wlpMsgLogPath

Changed into String[]

229e73e

turkeylurkey reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add constants for string literals in UI test classes #1199

Add constants for string literals in UI test classes #1199

anusreelakshmi934 commented Dec 18, 2024

dessina-devasia left a comment

anusreelakshmi934 commented Jan 2, 2025

mrglavas left a comment

dessina-devasia left a comment

anusreelakshmi934 commented Jan 14, 2025

TrevCraw left a comment

anusreelakshmi934 commented Jan 15, 2025

TrevCraw commented Jan 15, 2025

anusreelakshmi934 commented Jan 16, 2025

mrglavas commented Jan 16, 2025

mrglavas left a comment

TrevCraw left a comment

anusreelakshmi934 commented Jan 22, 2025

TrevCraw left a comment

TrevCraw Jan 22, 2025

anusreelakshmi934 Jan 23, 2025 •

edited

Loading

TrevCraw Jan 22, 2025

anusreelakshmi934 Jan 23, 2025

TrevCraw Jan 22, 2025

anusreelakshmi934 Jan 23, 2025

TrevCraw Jan 22, 2025

anusreelakshmi934 Jan 23, 2025

TrevCraw Jan 22, 2025

anusreelakshmi934 Jan 23, 2025

TrevCraw Jan 22, 2025

anusreelakshmi934 Jan 23, 2025

turkeylurkey Jan 23, 2025

Add constants for string literals in UI test classes #1199

Are you sure you want to change the base?

Add constants for string literals in UI test classes #1199

Conversation

anusreelakshmi934 commented Dec 18, 2024

dessina-devasia left a comment

Choose a reason for hiding this comment

anusreelakshmi934 commented Jan 2, 2025

mrglavas left a comment

Choose a reason for hiding this comment

dessina-devasia left a comment

Choose a reason for hiding this comment

anusreelakshmi934 commented Jan 14, 2025

TrevCraw left a comment

Choose a reason for hiding this comment

anusreelakshmi934 commented Jan 15, 2025

TrevCraw commented Jan 15, 2025

anusreelakshmi934 commented Jan 16, 2025

mrglavas commented Jan 16, 2025

mrglavas left a comment

Choose a reason for hiding this comment

TrevCraw left a comment

Choose a reason for hiding this comment

anusreelakshmi934 commented Jan 22, 2025

TrevCraw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusreelakshmi934 Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusreelakshmi934 Jan 23, 2025 •

edited

Loading