zentol commented on a change in pull request #16286:
URL: https://github.com/apache/flink/pull/16286#discussion_r662103266
##########
File path:
flink-clients/src/test/java/org/apache/flink/client/testjar/ClasspathProvider.java
##########
@@ -162,6 +164,7 @@ public void before() throws IOException {
directory = temporaryFolder.newFolder(directoryNameSuffix);
directoryContentCreator.accept(directory);
+ resetSystemClasspath();
Review comment:
it shouldn't be necessary to reset it in before()
##########
File path:
flink-clients/src/test/java/org/apache/flink/client/program/PackagedProgramRetrieverImplTest.java
##########
@@ -590,4 +567,25 @@ private JobGraph retrieveJobGraph(
packagedProgram, configuration, defaultParallelism,
false);
return PipelineExecutorUtils.getJobGraph(pipeline, configuration);
}
+
+ private static List<String>
extractRelativizedURLsForJarsFromDirectory(File directory)
+ throws MalformedURLException {
+ Preconditions.checkArgument(
+ directory.listFiles() != null,
+ "The passed File does not seem to be a directory: " +
directory.getAbsolutePath());
Review comment:
Why not use `File.isDirectory`?
##########
File path:
flink-clients/src/test/java/org/apache/flink/client/program/PackagedProgramRetrieverImplTest.java
##########
@@ -384,12 +384,22 @@ public void testEntryClassNotFoundOnUserClasspath()
throws FlinkException {
}
@Test(expected = FlinkException.class)
- public void testTooManyEntryClassesOnSystemClasspath() throws
FlinkException {
+ public void testWithoutJobClassAndMultipleEntryClassesOnUserClasspath()
throws FlinkException {
+ PackagedProgramRetrieverImpl.create(
+ multipleEntryClassesClasspathProvider.getDirectory(),
+ null,
+ new String[0],
+ new Configuration());
+ }
+
+ @Test(expected = FlinkException.class)
+ public void testWithoutJobClassAndMultipleEntryClassesOnSystemClasspath()
Review comment:
add a comment for why multiple entry classes are on the system classpath.
##########
File path:
flink-clients/src/test/java/org/apache/flink/client/program/PackagedProgramRetrieverImplTest.java
##########
@@ -126,10 +126,9 @@ public void
testDeriveEntryClassInformationForPythonBasedOnParameter() throws Fl
}
@Test
- public void testDeriveEntryClassInformationForCustomJar()
- throws FlinkException, MalformedURLException {
- // make loading from system classpath fail to make sure that it's not
triggered
- multipleEntryClassesClasspathProvider.setSystemClasspath();
+ public void testDeriveEntryClassInformationForCustomJar() throws
FlinkException {
+ // clearing the system classpath to make sure that no data is
collected from there
+ System.setProperty("java.class.path", "");
Review comment:
Could we not use the `noEntryClassClasspathProvider`? As is I'd argue
this test should explicitly restore the classpath property afterwards, and not
rely on the ClasspathProvider usages by other tests.
##########
File path:
flink-clients/src/test/java/org/apache/flink/client/program/PackagedProgramRetrieverImplTest.java
##########
@@ -197,8 +197,8 @@ public void
testDeriveEntryClassInformationFromClasspathWithJobClass()
PackagedProgramRetrieverImpl.createEntryClassInformationProvider(
// the user directory must be specified
multipleEntryClassesClasspathProvider.getDirectory(),
- // the user classpath is derived from the user
directory outside of the
- // method
+ // the user classpath is derived from the user
directory outside of
+ // createEntryClassInformationProvider
Review comment:
I still don't see the value of this comment. Isn't it obvious the
classpath is derived outside of createEntryClassInformationProvider if we pass
in the classpath?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
