sdedic commented on code in PR #7979: URL: https://github.com/apache/netbeans/pull/7979#discussion_r1869054872
########## ide/projectapi/src/org/netbeans/spi/project/ContainedProjectFilter.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.spi.project; + +import java.util.Collections; +import java.util.List; +import org.netbeans.api.project.Project; + +/** + * Review Comment: Pls. include some class overview and a description of an example usage (i.e. action acting on several projects may use this to filter). Should be noted that usage is optional and decided by individual actions and must be documented with the action. I see that in the implementation an empty ContentProjectFilter is treated as no filter; hypotetically (since this is an API) - is there a way how to express that an action working with this selector should operate on the project itself, not any of its children ? ########## java/gradle.test/src/org/netbeans/modules/gradle/test/GradleTestProgressListener.java: ########## @@ -131,9 +132,15 @@ private void processTestProgress(TestProgressEvent evt) { } } + @NbBundle.Messages({ + "ERR_Session_Null_Message=TestSession is null.", + }) private void processTestOutput(TestOutputEvent evt) { TestSession session = sessions.get(getSessionKey(evt.getDescriptor())); assert session != null; + if (session == null) { + throw new IllegalArgumentException(Bundle.ERR_Session_Null_Message()); Review Comment: Nitpick: IMHO no need to localize exceptons :) they'll probably result in an general error; and users rarely read the logfile. ########## java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/progress/TestProgressHandler.java: ########## @@ -154,7 +155,14 @@ private String statusToState(Status status) { 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; + String modulePath = getModuleTestPath(project); return new ModuleInfo(moduleName, modulePath); } + + private static String getModuleTestPath(Project project) { + if (project == null) { + return null; + } + return Paths.get(project.getProjectDirectory().getPath(), "src", "test", "java").toString(); Review Comment: Not sure about this -- @dbalek: shouldn't we rather use `Sources` API to get java/test source group and its folder from the project ? ########## ide/projectapi/src/org/netbeans/spi/project/ContainedProjectFilter.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.spi.project; + +import java.util.Collections; +import java.util.List; +import org.netbeans.api.project.Project; + +/** + * + * @author Dusan Petrovic + * + * @since 1.90 + */ +public final class ContainedProjectFilter { Review Comment: IMHO this should be in the `api` package: it's client facing, part of the "request', not implementation (SPI) - provided or produced by implementations. It's bad enough (an old sin) that clients access `spi.ActionProvider` directly ;) and not through an utility ########## java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/progress/ModuleInfo.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.java.lsp.server.progress; + +/** + * + * @author dusanpetrovic + */ +public class ModuleInfo { Review Comment: final ? ########## java/gradle.test/src/org/netbeans/modules/gradle/test/GradleTestProgressListener.java: ########## @@ -241,11 +259,38 @@ private void caseFinish(TestFinishEvent evt, JvmTestOperationDescriptor op) { } } - runningTests.remove(getTestOpKey(op)); } } + private static String GRADLE_TEST_RUN = "Gradle Test Run :"; Review Comment: nitpick: make final :), `// NOI18N` ########## java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/debugging/launch/NbLaunchDelegate.java: ########## @@ -76,6 +76,7 @@ import org.netbeans.modules.nativeimage.api.debug.StartDebugParameters; import org.netbeans.spi.project.ActionProgress; import org.netbeans.spi.project.ActionProvider; +import org.netbeans.spi.project.ContainedProjectFilter; Review Comment: bump dependency version for project.api ########## ide/projectapi/apichanges.xml: ########## @@ -83,6 +83,18 @@ is the proper place. <!-- ACTUAL CHANGES BEGIN HERE: --> <changes> + <change id="contained-project-context"> Review Comment: Pls mention ActionProvider's change as well. ########## java/maven/src/org/netbeans/modules/maven/api/execute/RunConfig.java: ########## @@ -79,6 +79,28 @@ 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(); Review Comment: Friend API change; probably add `apichanges.xml` entry for this + bump version of the module. ########## java/maven/src/org/netbeans/modules/maven/ActionProviderImpl.java: ########## @@ -132,6 +132,7 @@ public class ActionProviderImpl implements ActionProvider { "javadoc", //NOI18N COMMAND_TEST, COMMAND_TEST_SINGLE, + COMMAND_TEST_PARALLEL, Review Comment: bump dependency version on project.api ########## java/gradle.test/src/org/netbeans/modules/gradle/test/GradleTestProgressListener.java: ########## @@ -164,7 +171,7 @@ private void sessionStart(TestStartEvent evt) { } } - private void sessionFinish(TestFinishEvent evt) { + private synchronized void sessionFinish(TestFinishEvent evt) { Review Comment: I briefly checked the implementation of CoreManager.sessionFInished() called below, and it seems that the impl uses Mutex.EVENT.writeAccess that blocks until the action is executed in EDT thread. This `synchronized` lock will be held for the whole time, incl. waiting on EDT; may lead to deadlock eventually. Same for other CoreManager etc calls that may fire events to unknown listeners or synchronize with other threads. ########## ide/projectapi/src/org/netbeans/spi/project/ActionProvider.java: ########## @@ -83,6 +83,11 @@ public interface ActionProvider { */ String COMMAND_TEST_SINGLE = "test.single"; // NOI18N + /** + * Standard command for running tests in parallel on given projects sub-modules + */ + String COMMAND_TEST_PARALLEL = "test.parallel"; // NOI18N Review Comment: `@since` missing ########## java/maven/src/org/netbeans/modules/maven/execute/MavenCommandLineExecutor.java: ########## @@ -446,21 +451,24 @@ private static List<String> createMavenExecutionCommand(RunConfig config, Constr } } + //#164234 + //if maven.bat file is in space containing path, we need to quote with simple quotes. + String quote = "\""; + 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 + if (MavenCommandLineOptions.optionRequiresValue(key) && (value.isEmpty() || value.equals("${" + key + "}"))) { //NOI18N + DialogDisplayer.getDefault().notify(new NotifyDescriptor.Message(Bundle.MSG_MissingValue(key), NotifyDescriptor.WARNING_MESSAGE)); Review Comment: `notify` will suspend the caller (launching process) until the user confirms / closes the message. Consider using `notifyLater` if that's not the intent. ########## java/gradle.java/src/org/netbeans/modules/gradle/java/JavaActionProvider.java: ########## @@ -72,6 +72,7 @@ public class JavaActionProvider extends DefaultGradleActionsProvider { COMMAND_RUN, COMMAND_DEBUG, COMMAND_TEST, + COMMAND_TEST_PARALLEL, Review Comment: check the project's dependency on `project.api`, make sure it requests 1.90 where the symbol is added. Increase this module's spec version (feature added). ########## java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/WorkspaceServiceImpl.java: ########## @@ -805,6 +808,13 @@ public CompletableFuture<Object> executeCommand(ExecuteCommandParams params) { throw new UnsupportedOperationException("Command not supported: " + params.getCommand()); } + private String getModuleTestPath(Project project) { + if (project == null) { + return null; + } + return Paths.get(project.getProjectDirectory().getPath(), "src", "test", "java").toString(); Review Comment: similar for the usage of Source API - whatever decision is taken. ########## java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/progress/TestProgressHandler.java: ########## @@ -144,4 +149,20 @@ private String statusToState(Status status) { throw new IllegalStateException("Unexpected testsuite status: " + status); } } + + private static Pattern PATTERN = Pattern.compile("Gradle Test Run :(.*):test"); Review Comment: make final, // NOI18N -- 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
