XComp commented on a change in pull request #16286:
URL: https://github.com/apache/flink/pull/16286#discussion_r662085256
##########
File path:
flink-clients/src/test/java/org/apache/flink/client/program/PackagedProgramRetrieverImplTest.java
##########
@@ -328,18 +329,15 @@ public void testFailIfJobDirDoesNotHaveEntryClass() {
}
}
- @Test
- public void testEntryClassNotFoundOnSystemClasspath() throws
FlinkException, IOException {
- classpathProvider.setSystemClasspathWithoutEntryClass();
- final PackagedProgramRetrieverImpl retriever =
- PackagedProgramRetrieverImpl.create(
- null,
- classpathProvider.getTestJobClassName(),
- new String[0],
- new Configuration());
- assertThat(
- retriever.getPackagedProgram().getMainClassName(),
- is(classpathProvider.getTestJobClassName()));
+ // TODO: this test checks the same code path as
+ //
testDeriveEntryClassInformationFromSystemClasspathWithNonExistingJobClassName
+ // We should make it fail early if the class is not present on the system
classpath
+ // Right now, the test is failing because no error is thrown
+ @Ignore
+ @Test(expected = FlinkException.class)
+ public void testEntryClassNotFoundOnSystemClasspath() throws
FlinkException {
Review comment:
Correct, but I would keep it in as an additional test in case we decide
to make the code more strict (as mentioned in [this
comment](https://github.com/apache/flink/pull/16286#discussion_r658542629)).
Right now, I lean towards keeping it like it was implemented in the old
implementation to not open any hidden problems which might make the PR become
even bigger. Instead, I'd create a Flink issue to resolve that. But I wanted to
get your opinions on that one. I wasn't 100% sure whether it's actually a
desired behavior not checking the class existence on the system 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
