sdedic commented on code in PR #7979:
URL: https://github.com/apache/netbeans/pull/7979#discussion_r1863420851
##########
extide/gradle/src/org/netbeans/modules/gradle/ActionProviderImpl.java:
##########
@@ -215,6 +214,9 @@ static String taskName(Project project, String action,
Lookup lkp) {
case ActionProvider.COMMAND_TEST:
title = TXT_Test(prjLabel);
break;
+ case ActionProvider.COMMAND_TEST_PARALLEL:
+ title = TXT_Test(prjLabel);
Review Comment:
Different label from COMMAND_TEST, please
##########
java/java.lsp.server/nbcode/integration/src/org/netbeans/modules/nbcode/integration/maven-actions-override.xml:
##########
@@ -55,7 +55,23 @@
<test>${packageClassName}</test>
</properties>
</action>
-
+ <action>
Review Comment:
Is there a reason the action mapping is defined in the NBLS `integration`
module, while the action's implementation iself is in `java/maven` standard
distribution ?
##########
java/maven/src/org/netbeans/modules/maven/execute/MavenCommandLineExecutor.java:
##########
@@ -445,6 +445,18 @@ private static List<String>
createMavenExecutionCommand(RunConfig config, Constr
LOGGER.log(Level.FINE, "Could not canonicalize " + basedir, x);
}
}
+
+ for (Map.Entry<? extends String, ? extends String> entry :
config.getOptions().entrySet()) {
+ String key = entry.getKey();
+ String value = quote2apos(entry.getValue());
+ if (MavenCommandLineOptions.optionRequiresValue(key) &&
(value.isBlank() || value.equals("${" + key + "}"))) { //NOI18N
Review Comment:
why the check for `${optionName}` ?
Consider to print a warning visibly to the user: if trying to pass no value
for an option that is known to have one, it's a bug somewhere in the code.
Maybe I am picky, but `isBlank()` is true also for " " (two spaces); I
wouldn't equal that to a no-value. Consider value quoting (see proprerties
below).
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/debugging/launch/NbLaunchDelegate.java:
##########
@@ -374,6 +377,9 @@ private static ExplicitProcessParameters
createExplicitProcessParameters(Map<Str
if (!env.isEmpty()) {
bld.environmentVariables(env);
}
+ if (!projects.isEmpty()) {
Review Comment:
So, the goal here is to pass list of projects the **project action** should
apply to, rather than configure the launched application; the rest of
ExplicitProcessParameters is used to setup environment for the executed VM.
We do not have much support for "umbrella" actions in container projects at
this moment; so it's maybe better to introduce such explicit support at
`projects.api` level, given that the concept of `ProjectContainerProvider` is
here.
A simple bean like
```
public final class ContainedProjectFilter {
private List<Project> projectsToProcess;
}
```
would be sufficient, and usable (possibly!) for other actions. I thought
about an `Predicate<Project>`, but the caller can inspect the project structure
in advance and generate explicit information which is easier to process during
build.
The bean should be final with a nonpublic constructor + static factory, so
that it can be easily extended in the future (e.g. with a Builder).
##########
java/gradle.test/src/org/netbeans/modules/gradle/test/GradleTestProgressListener.java:
##########
@@ -131,17 +132,19 @@ private void processTestProgress(TestProgressEvent evt) {
}
private void processTestOutput(TestOutputEvent evt) {
+ TestSession session = sessions.get(getSessionKey(evt.getDescriptor()));
+ assert session != null;
Review Comment:
Maybe fail with IAE even in production mode if `session == null`; otherwise
invent a way how the output will be displayed and/or reported even in that case.
The original code would IMHO fail if sessionStart wouldn't be run first.
##########
java/gradle.test/src/org/netbeans/modules/gradle/test/GradleTestProgressListener.java:
##########
@@ -241,11 +252,38 @@ private void caseFinish(TestFinishEvent evt,
JvmTestOperationDescriptor op) {
}
}
- runningTests.remove(getTestOpKey(op));
}
}
+ private static String GRADLE_TEST_RUN = "Gradle Test Run :";
+ private static String TEST = ":test";
+
+ private Project getProject(String key) {
+ if (key != null && key.startsWith(GRADLE_TEST_RUN)) {
+ key = key.substring(GRADLE_TEST_RUN.length());
+ if (key.endsWith(TEST)) {
+ key = key.substring(0, key.length() - TEST.length()).trim();
+ if (!key.isEmpty()) {
+ for (Project containedPrj :
ProjectUtils.getContainedProjects(project, false)) {
+ if
(key.equals(containedPrj.getProjectDirectory().getName())) {
+ return containedPrj;
+ }
+ }
+ }
+ }
+ }
+ return project;
+ }
+
+ private static String getSessionKey(OperationDescriptor op) {
+ String id = "";
+ for (OperationDescriptor descriptor = op; descriptor != null;
descriptor = descriptor.getParent()) {
+ id = descriptor.getName();
Review Comment:
Just checking: Is session key the topmost (root) operation's name ?
##########
java/gradle.test/src/org/netbeans/modules/gradle/test/GradleTestProgressListener.java:
##########
@@ -151,8 +154,9 @@ private void processTestOutput(TestOutputEvent evt) {
private void sessionStart(TestStartEvent evt) {
- session = new TestSession(evt.getDisplayName(), project,
TestSession.SessionType.TEST);
- runningTests.clear();
+ String key = getSessionKey(evt.getDescriptor());
Review Comment:
Please check that the event delivery dispatched into `*Start / *Stop`
methods is serialized, there are now 2 Maps (sessions + runningTests 1.st
level) that work together and their mutations are not atomic. Otherwise these
two may need synchronizing.
##########
ide/extexecution.base/src/org/netbeans/api/extexecution/base/ExplicitProcessParameters.java:
##########
@@ -146,6 +148,16 @@ public boolean isEmpty() {
public List<String> getArguments() {
return arguments;
}
+
+ /**
+ * Returns the projects to be passed. Returns {@code null} if the object
does not
+ * want to alter the project list.
+ * @return projects to be passed or {@code null} if the project list
should not be altered.
+ */
+ public List<String> getProjects() {
Review Comment:
Sorry, this is not acceptable. `extexecution.base` is project-agnostic and
should remain that way. This whole support API arguments for a final launcher,
VM or executable that is to be run "somehow", maybe through a project action or
a build system, but also independently.
This PR is not trying to pass projects to the user application, its VM or a
potential wrapping launcher, but rather to the build system; it does not affect
the user's VM/launch setup.
##########
java/gradle.test/src/org/netbeans/modules/gradle/test/GradleTestProgressListener.java:
##########
@@ -241,11 +252,38 @@ private void caseFinish(TestFinishEvent evt,
JvmTestOperationDescriptor op) {
}
}
- runningTests.remove(getTestOpKey(op));
}
}
+ private static String GRADLE_TEST_RUN = "Gradle Test Run :";
+ private static String TEST = ":test";
+
+ private Project getProject(String key) {
+ if (key != null && key.startsWith(GRADLE_TEST_RUN)) {
+ key = key.substring(GRADLE_TEST_RUN.length());
+ if (key.endsWith(TEST)) {
+ key = key.substring(0, key.length() - TEST.length()).trim();
+ if (!key.isEmpty()) {
+ for (Project containedPrj :
ProjectUtils.getContainedProjects(project, false)) {
Review Comment:
is it possible that toplevel `test` executes tests on 3rd level projects as
well ? In that case, you might need contained projects recursively.
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/progress/TestProgressHandler.java:
##########
@@ -144,4 +148,13 @@ private String statusToState(Status status) {
throw new IllegalStateException("Unexpected testsuite status:
" + status);
}
}
+
+ private static Pattern PATTERN = Pattern.compile("Gradle Test Run
:(.*):test");
+
+ private static ModuleInfo getModuleInfo(TestSession session) {
+ Project project = session.getProject();
+ String moduleName = project != null ?
ProjectUtils.getInformation(project).getDisplayName() : null;
+ String modulePath = project != null ?
project.getProjectDirectory().getPath() : null;
Review Comment:
Not sure about this; subproject information is usually relative to the root
project. Is it desirable to have full FS path here ?
##########
java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/debugging/launch/NbLaunchDelegate.java:
##########
@@ -568,7 +574,8 @@ public void finished(boolean success) {
actions = debug ? mainSource ? new String[]
{ActionProvider.COMMAND_DEBUG_SINGLE}
: new String[]
{ActionProvider.COMMAND_DEBUG_TEST_SINGLE, ActionProvider.COMMAND_DEBUG_SINGLE}
: mainSource ? new String[]
{ActionProvider.COMMAND_RUN_SINGLE}
- : new String[]
{ActionProvider.COMMAND_TEST_SINGLE, ActionProvider.COMMAND_RUN_SINGLE};
+ : testInParallel ? new String[]
{ActionProvider.COMMAND_TEST_PARALLEL, ActionProvider.COMMAND_TEST}
Review Comment:
Why the difference if `testInParallel` ?
COMMAND_TEST_**SINGLE** vs. COMMAND_TEST_PARALLEL (a clone of COMMAND_TEST
not *single* actually) and COMMAND_**TEST** vs COMMAND_**RUN_SINGLE** ?
##########
java/java.lsp.server/vscode/src/testAdapter.ts:
##########
@@ -265,14 +302,42 @@ export class NbTestAdapter {
});
} else {
currentSuite.children.replace(children);
+ currentModule.children.replace(suiteChildren);
}
}
+ getModuleName(suite: TestSuite): string {
+ return suite.moduleName?.replace(":", "-") || "";
+ }
+
+ getModulePath(suite: TestSuite): Uri {
+ const basePath = Uri.parse(suite.modulePath || "");
+ return Uri.joinPath(basePath, "src", "test", "java");
Review Comment:
I woudn't hardcode any path inside a project, better have NBLS to report the
path to the LS client.
##########
java/java.lsp.server/vscode/src/testAdapter.ts:
##########
@@ -198,19 +213,41 @@ export class NbTestAdapter {
} else {
this.set(currentSuite, suite.state, undefined,
true);
}
+ this.set(currentModule,
this.calculateStateFor(currentModule), undefined, true);
}
}
break;
}
}
+ calculateStateFor(testItem: TestItem): 'passed' | 'failed' | 'skipped' |
'errored' {
+ let passed: number = 0;
+ testItem.children.forEach(item => {
+ const state = this.suiteStates.get(item);
+ if (state === 'enqueued' || state === 'failed') return state;
+ if (state === 'passed') passed++;
+ })
+ if (passed > 0) return 'passed';
Review Comment:
what about `started` state ? Shouldn't the module indicate somehow it is
still running (although still passing, or already failed) ?
##########
java/maven/src/org/netbeans/modules/maven/api/execute/RunConfig.java:
##########
@@ -79,6 +79,17 @@ public interface RunConfig {
String getActionName();
+ /**
+ * Options/switches passed to maven.
+ * @return a read-only copy of the current maven options
+ * @since 2.166
+ */
+ @NonNull Map<? extends String,? extends String> getOptions();
+
+ void setOptions(@NonNull String key, @NullAllowed String value);
Review Comment:
`setOptio`n`` or `addOption` :) no multple key-values here.
Pls. document all added methods.
##########
java/maven/src/org/netbeans/modules/maven/ActionProviderImpl.java:
##########
@@ -457,6 +459,8 @@ private static void setupTaskName(String action, RunConfig
config, Lookup lkp) {
title = TXT_Profile(prjLabel);
} else if (ActionProvider.COMMAND_TEST.equals(action)) {
title = TXT_Test(prjLabel);
+ } else if (ActionProvider.COMMAND_TEST_PARALLEL.equals(action)) {
+ title = TXT_Test(prjLabel);
Review Comment:
Different title from COMMAND_TEST
##########
java/maven/src/org/netbeans/modules/maven/execute/AbstractOutputHandler.java:
##########
@@ -226,12 +232,16 @@ protected final void processEnd(String id, OutputWriter
writer) {
writer.println(visitor.getLine());
}
}
- Set set = processors.get(id);
- if (set != null) {
- //TODO a bulletproof way would be to keep a list of currently
started
- // sections and compare to the list of
getRegisteredOutputSequences fo each of the
- // processors in set..
- currentProcessors.removeAll(set);
+ AtomicInteger count = id2count.getOrDefault(id, new AtomicInteger(1));
+ if (count.decrementAndGet() == 0) {
Review Comment:
nitpick: add+remove AtomicInteger to the map on missing `id`; consider using
just `count = id2count.remove();` then `count == null ||
count.decrementAndGet())` as test.
##########
java/maven/src/org/netbeans/modules/maven/execute/MavenCommandLineOptions.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.netbeans.modules.maven.execute;
+
+import java.util.List;
+
+/**
+ *
+ * @author Dusan Petrovic
+ */
+public class MavenCommandLineOptions {
+
+ private static final List<String> OPTIONS_WITH_VALUES = List.of(
+ "file",
Review Comment:
Also
- builder
- color
- define
- global-toolchains
Pls.order alphabetically in source, it is easier to find an option when
reading :) I'd use `Set.of`, faster `contains` lookups.
##########
java/maven.junit/src/org/netbeans/modules/maven/junit/JUnitOutputListenerProvider.java:
##########
@@ -632,25 +624,21 @@ static Trouble constructTrouble(@NonNull String type,
@NullAllowed String messag
}
private void generateTest() {
- String suffix = reportNameSuffix;
- if (suffix == null) {
- suffix = "";
- } else {
- //#204480
- suffix = "-" + suffix;
- }
- File report = new File(outputDir, "TEST-" + runningTestClass + suffix
+ ".xml");
- if (!report.isFile() || report.lastModified() < startTimeStamp) {
//#219097 ignore results from previous invokation.
- if(surefireRunningInParallel) { // try waiting a bit to give time
for the result file to be created
- try {
- Thread.sleep(500);
- } catch (InterruptedException ex) {
- Exceptions.printStackTrace(ex);
- }
- }
- if (!report.isFile() || report.lastModified() < startTimeStamp) {
// and now try again
- return;
+ String suffix = reportNameSuffix == null ? "" : "-" + reportNameSuffix;
+ File outputDir = locateOutputDir(suffix);
+ if (outputDir == null && surefireRunningInParallel) {
+ // try waiting a bit to give time for the result file to be created
Review Comment:
Maybe there could be a warning message to the user if the output is not
created even after the wait.
##########
java/maven/src/org/netbeans/modules/maven/execute/cmd/ExecMojo.java:
##########
@@ -155,4 +169,24 @@ public String getImplementationClass() {
void setImplementationClass(String implementationClass) {
this.implementationClass = implementationClass;
}
+
+ public @CheckForNull Project findProject() {
+ if (currentProjectLocation != null) {
+ FileObject fo = FileUtil.toFileObject(currentProjectLocation);
+ if (fo != null) {
+ try {
+ return ProjectManager.getDefault().findProject(fo);
+ } catch (IOException ex) {
+ //Exceptions.printStackTrace(ex);
Review Comment:
Why this should not be logged ?
##########
java/maven/src/org/netbeans/modules/maven/execute/model/NetbeansActionMapping.java:
##########
@@ -264,6 +290,16 @@ public void removePackaging(String string)
getPackagings().remove( string );
} //-- void removePackaging(String)
+ /**
+ * Method removeOptions.
+ *
+ * @param string
+ */
+ public void removeOptions(String string)
+ {
Review Comment:
singular `removeOption` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
