RFR: 7903965: Fix Launching Tests from Context Menu
Jorn Vernee
jvernee at openjdk.org
Mon Mar 10 12:59:52 UTC 2025
On Wed, 5 Mar 2025 17:28:47 GMT, Oleksii Sylichenko <duke at openjdk.org> wrote:
> Currently, there is an issue with running TestNG/JUnit tests when using the context menu on the editor tab. TestNG/JUnit intercepts the test execution from JTReg.
>
> After fixing this issue, I decided to extend the fix to cover all potential use cases of the context menu.
>
> As part of this pull request, I propose enhancing the process of running tests via the context menu. The context menu can be used in the following cases:
>
> - Editor tab
> - File in the project tree
> - Directory in the project tree
> - Any location within a file
>
> The current changes automatically determine what needs to be executed:
>
> - Directory
> - File
> - Method
>
> Additionally, the issue with intercepting test execution through Application, TestNG, and JUnit configurations when using the context menu on the editor tab has been fixed.
>
> # Summary
>
> - Ensure that run configuration priority is no longer lost to Application, TestNG, or JUnit setups when launching via the context menu.
> - Fix run configuration created via the context menu: find and launch the proper test element, depending on the current PSI element.
> - Improve the logic for comparing existing run configurations with newly generated ones.
> - Add JUnit plugin in the dependencies.
>
> # Detailed changes
>
> ## Add JUnit plugin into dependencies
> - build.gradle
> - plugin.xml
>
> ## JTRegClassConfigurationProducer.java
> - Refactor `setupConfigurationFromContext`:
> - Move the check for whether a file can be run to `JTRegUtils#isRunnableByJTReg`.
> - Allow this method to create configurations for directories as well.
> - Implement `preventRunPriorityLoss` to prevent priority takeover by other run configuration types (Application, TestNG, JUnit) under certain conditions.
> - Replace the use of the current PSI element for generating names and querying run configurations with the specific element intended for execution.
> - Implement `preventRunPriorityLoss`.
> - Add Javadoc for `nameForElement`.
>
> ## JTRegConfigurationProducer.java
> - Fix logic in `isConfigurationFromContext` for checking if two configurations are identical:
> - Replace the use of the current PSI element for queries with the exact element intended for execution.
> - Compare string-typed configuration parameters using a "not-nullify" approach (`null == ""`).
> - Implement `findExactRunElement` to locate the specific PSI element for execution among the current element’s parents.
> - Add a JUnit check to the `shouldReplace` method.
>
> ## JTRegUtils.java
> - Add a predicate to compare two strings ...
I've created https://bugs.openjdk.org/browse/CODETOOLS-7903965 for this PR
plugins/idea/src/main/java/com/oracle/plugin/jtreg/configuration/producers/JTRegClassConfigurationProducer.java line 73:
> 71: if (element instanceof PsiDirectory runDir) {
> 72: configuration.setPackage(runDir.getVirtualFile().getPath());
> 73: } else {
If this producer can now handle directories too, do we still need `JTRegDirectoryConfigurationProducer`?
Also, for a directory this would end up calling `nameForElement` -> `element.getContainingFile().getName()`, where `getContainingFile` seemingly returns `null`, so I think that would fail.
plugins/idea/src/main/java/com/oracle/plugin/jtreg/configuration/producers/JTRegClassConfigurationProducer.java line 125:
> 123: }
> 124: }
> 125: }
> Additionally, the issue with intercepting test execution through Application, TestNG, and JUnit configurations when using the context menu on the editor tab has been fixed.
I think this issue is addressed in this code here.
What are the steps to reproduce this issue? When I try to create a run configuration through the context menu on the editor tab, I seem to get a JTreg configuration every time. Or I get the option to choose one or the other:
plugins/idea/src/main/java/com/oracle/plugin/jtreg/configuration/producers/JTRegConfigurationProducer.java line 76:
> 74: }
> 75:
> 76: final Location<?> location = JavaExecutionUtil.stepIntoSingleClass(contextLocation);
Suggestion:
Location<?> location = JavaExecutionUtil.stepIntoSingleClass(contextLocation);
plugins/idea/src/main/java/com/oracle/plugin/jtreg/configuration/producers/JTRegConfigurationProducer.java line 90:
> 88: final String contextFilePath = null != contextVirtualFile ? contextVirtualFile.getPath() : null;
> 89:
> 90: final String contextDirPath = (element instanceof PsiDirectory d) ? d.getVirtualFile().getPath() : null;
Suggestion:
String contextQuery = getQuery(element);
PsiFile contextFile = (element instanceof PsiFile psiFile) ? psiFile : element.getContainingFile();
VirtualFile contextVirtualFile = null != contextFile ? contextFile.getVirtualFile() : null;
String contextFilePath = null != contextVirtualFile ? contextVirtualFile.getPath() : null;
String contextDirPath = (element instanceof PsiDirectory d) ? d.getVirtualFile().getPath() : null;
plugins/idea/src/main/java/com/oracle/plugin/jtreg/configuration/producers/JTRegConfigurationProducer.java line 128:
> 126:
> 127: if (IS_THIRD_PARTY_TEST_ELEMENT.test(e)) {
> 128: final PsiElement identifyingElement = ((PsiNameIdentifierOwner) e).getIdentifyingElement();
Suggestion:
PsiElement identifyingElement = ((PsiNameIdentifierOwner) e).getIdentifyingElement();
plugins/idea/src/main/java/com/oracle/plugin/jtreg/configuration/producers/JTRegConfigurationProducer.java line 131:
> 129: if (null != identifyingElement) { // null for name of the non-test inner class
> 130: if (null == retval) {
> 131: // When found, check the left hierarchy up to the class for runnability
Suggestion:
// When found, check the rest of the hierarchy up to the class for runnability
plugins/idea/src/main/java/com/oracle/plugin/jtreg/configuration/producers/JTRegConfigurationProducer.java line 233:
> 231: @Override
> 232: public boolean shouldReplace(@NotNull ConfigurationFromContext self, @NotNull ConfigurationFromContext other) {
> 233: final RunConfiguration otherCnf = other.getConfiguration();
Suggestion:
RunConfiguration otherCnf = other.getConfiguration();
plugins/idea/src/main/java/com/oracle/plugin/jtreg/util/JTRegUtils.java line 94:
> 92: public static final Predicate<PsiElement> IS_DIR_ELEMENT = PsiDirectory.class::isInstance;
> 93: public static final Predicate<PsiElement> IS_FILE_ELEMENT = PsiFile.class::isInstance;
> 94: public static final Predicate<PsiElement> IS_FILE_OR_DIR_ELEMENT = IS_FILE_ELEMENT.or(IS_DIR_ELEMENT);
Why are these all predicates? I'd much prefer a set of static methods. These predicates make the logic harder to follow.
plugins/idea/src/main/java/com/oracle/plugin/jtreg/util/JTRegUtils.java line 355:
> 353: public static boolean isRunnableByJTReg(@NotNull PsiElement element) {
> 354: final PsiFile runFile;
> 355: final PsiDirectory runDir;
Suggestion:
PsiFile runFile;
PsiDirectory runDir;
plugins/idea/src/main/resources/META-INF/plugin.xml line 36:
> 34: <depends>AntSupport</depends>
> 35: <depends>TestNG-J</depends>
> 36: <depends>JUnit</depends>
Is this required for `JUnitUtil` and `JUnitConfiguration`?
-------------
PR Comment: https://git.openjdk.org/jtreg/pull/252#issuecomment-2710465198
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987145313
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987159096
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987163320
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987163649
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987170313
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987170095
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987135751
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987168370
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987134337
PR Review Comment: https://git.openjdk.org/jtreg/pull/252#discussion_r1987132790
More information about the jtreg-dev
mailing list