flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (FLINK-8340) Do not pass Configuration and configuration directory to CustomCommandLine methods
Date Fri, 05 Jan 2018 14:13:00 GMT

    [ https://issues.apache.org/jira/browse/FLINK-8340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16313166#comment-16313166
] 

ASF GitHub Bot commented on FLINK-8340:
---------------------------------------

Github user GJL commented on a diff in the pull request:

    https://github.com/apache/flink/pull/5226#discussion_r159883585
  
    --- Diff: flink-clients/src/test/java/org/apache/flink/client/cli/CliFrontendPackageProgramTest.java
---
    @@ -60,220 +59,178 @@ public static void init() {
     	}
     
     	@Test
    -	public void testNonExistingJarFile() {
    -		try {
    -			CliFrontend frontend = new CliFrontend(
    -				new Configuration(),
    -				Collections.singletonList(new DefaultCLI()),
    -				CliFrontendTestUtils.getConfigDir());
    +	public void testNonExistingJarFile() throws Exception {
    +		Configuration configuration = new Configuration();
    +		CliFrontend frontend = new CliFrontend(
    +			configuration,
    +			Collections.singletonList(new DefaultCLI(configuration)));
     
    -			ProgramOptions options = mock(ProgramOptions.class);
    -			when(options.getJarFilePath()).thenReturn("/some/none/existing/path");
    +		ProgramOptions options = mock(ProgramOptions.class);
    +		when(options.getJarFilePath()).thenReturn("/some/none/existing/path");
     
    -			try {
    -				frontend.buildProgram(options);
    -				fail("should throw an exception");
    -			}
    -			catch (FileNotFoundException e) {
    -				// that's what we want
    -			}
    +		try {
    +			frontend.buildProgram(options);
    +			fail("should throw an exception");
     		}
    -		catch (Exception e) {
    -			e.printStackTrace();
    -			fail(e.getMessage());
    +		catch (FileNotFoundException e) {
    +			// that's what we want
     		}
     	}
     
     	@Test
    -	public void testFileNotJarFile() {
    -		try {
    -			CliFrontend frontend = new CliFrontend(
    -				new Configuration(),
    -				Collections.singletonList(new DefaultCLI()),
    -				CliFrontendTestUtils.getConfigDir());
    +	public void testFileNotJarFile() throws Exception {
    +		Configuration configuration = new Configuration();
    +		CliFrontend frontend = new CliFrontend(
    +			configuration,
    +			Collections.singletonList(new DefaultCLI(configuration)));
     
    -			ProgramOptions options = mock(ProgramOptions.class);
    -			when(options.getJarFilePath()).thenReturn(getNonJarFilePath());
    +		ProgramOptions options = mock(ProgramOptions.class);
    +		when(options.getJarFilePath()).thenReturn(getNonJarFilePath());
     
    -			try {
    -				frontend.buildProgram(options);
    -				fail("should throw an exception");
    -			}
    -			catch (ProgramInvocationException e) {
    -				// that's what we want
    -			}
    +		try {
    +			frontend.buildProgram(options);
    +			fail("should throw an exception");
     		}
    -		catch (Exception e) {
    -			e.printStackTrace();
    -			fail(e.getMessage());
    +		catch (ProgramInvocationException e) {
    +			// that's what we want
     		}
     	}
     
     	@Test
    -	public void testVariantWithExplicitJarAndArgumentsOption() {
    -		try {
    -			String[] arguments = {
    -					"--classpath", "file:///tmp/foo",
    -					"--classpath", "file:///tmp/bar",
    -					"-j", getTestJarPath(),
    -					"-a", "--debug", "true", "arg1", "arg2" };
    -			URL[] classpath = new URL[] { new URL("file:///tmp/foo"), new URL("file:///tmp/bar")
};
    -			String[] reducedArguments = new String[] {"--debug", "true", "arg1", "arg2"};
    -
    -			RunOptions options = CliFrontendParser.parseRunCommand(arguments);
    -			assertEquals(getTestJarPath(), options.getJarFilePath());
    -			assertArrayEquals(classpath, options.getClasspaths().toArray());
    -			assertArrayEquals(reducedArguments, options.getProgramArgs());
    -
    -			CliFrontend frontend = new CliFrontend(
    -				new Configuration(),
    -				Collections.singletonList(new DefaultCLI()),
    -				CliFrontendTestUtils.getConfigDir());
    -			PackagedProgram prog = frontend.buildProgram(options);
    +	public void testVariantWithExplicitJarAndArgumentsOption() throws Exception {
    +		String[] arguments = {
    +				"--classpath", "file:///tmp/foo",
    +				"--classpath", "file:///tmp/bar",
    +				"-j", getTestJarPath(),
    +				"-a", "--debug", "true", "arg1", "arg2" };
    +		URL[] classpath = new URL[] { new URL("file:///tmp/foo"), new URL("file:///tmp/bar")
};
    +		String[] reducedArguments = new String[] {"--debug", "true", "arg1", "arg2"};
    +
    +		RunOptions options = CliFrontendParser.parseRunCommand(arguments);
    +		assertEquals(getTestJarPath(), options.getJarFilePath());
    +		assertArrayEquals(classpath, options.getClasspaths().toArray());
    +		assertArrayEquals(reducedArguments, options.getProgramArgs());
    +
    +		Configuration configuration = new Configuration();
    +		CliFrontend frontend = new CliFrontend(
    +			configuration,
    +			Collections.singletonList(new DefaultCLI(configuration)));
    +		PackagedProgram prog = frontend.buildProgram(options);
     
    -			Assert.assertArrayEquals(reducedArguments, prog.getArguments());
    -			Assert.assertEquals(TEST_JAR_MAIN_CLASS, prog.getMainClassName());
    -		}
    -		catch (Exception e) {
    -			e.printStackTrace();
    -			fail(e.getMessage());
    -		}
    +		Assert.assertArrayEquals(reducedArguments, prog.getArguments());
    +		Assert.assertEquals(TEST_JAR_MAIN_CLASS, prog.getMainClassName());
     	}
     
     	@Test
    -	public void testVariantWithExplicitJarAndNoArgumentsOption() {
    -		try {
    -			String[] arguments = {
    -					"--classpath", "file:///tmp/foo",
    -					"--classpath", "file:///tmp/bar",
    -					"-j", getTestJarPath(),
    -					"--debug", "true", "arg1", "arg2" };
    -			URL[] classpath = new URL[] { new URL("file:///tmp/foo"), new URL("file:///tmp/bar")
};
    -			String[] reducedArguments = new String[] {"--debug", "true", "arg1", "arg2"};
    -
    -			RunOptions options = CliFrontendParser.parseRunCommand(arguments);
    -			assertEquals(getTestJarPath(), options.getJarFilePath());
    -			assertArrayEquals(classpath, options.getClasspaths().toArray());
    -			assertArrayEquals(reducedArguments, options.getProgramArgs());
    -
    -			CliFrontend frontend = new CliFrontend(
    -				new Configuration(),
    -				Collections.singletonList(new DefaultCLI()),
    -				CliFrontendTestUtils.getConfigDir());
    +	public void testVariantWithExplicitJarAndNoArgumentsOption() throws Exception {
    +		String[] arguments = {
    +				"--classpath", "file:///tmp/foo",
    +				"--classpath", "file:///tmp/bar",
    +				"-j", getTestJarPath(),
    +				"--debug", "true", "arg1", "arg2" };
    +		URL[] classpath = new URL[] { new URL("file:///tmp/foo"), new URL("file:///tmp/bar")
};
    +		String[] reducedArguments = new String[] {"--debug", "true", "arg1", "arg2"};
    +
    +		RunOptions options = CliFrontendParser.parseRunCommand(arguments);
    +		assertEquals(getTestJarPath(), options.getJarFilePath());
    +		assertArrayEquals(classpath, options.getClasspaths().toArray());
    +		assertArrayEquals(reducedArguments, options.getProgramArgs());
    +
    +		Configuration configuration = new Configuration();
    +		CliFrontend frontend = new CliFrontend(
    +			configuration,
    +			Collections.singletonList(new DefaultCLI(configuration)));
     
    -			PackagedProgram prog = frontend.buildProgram(options);
    +		PackagedProgram prog = frontend.buildProgram(options);
     
    -			Assert.assertArrayEquals(reducedArguments, prog.getArguments());
    -			Assert.assertEquals(TEST_JAR_MAIN_CLASS, prog.getMainClassName());
    -		}
    -		catch (Exception e) {
    -			e.printStackTrace();
    -			fail(e.getMessage());
    -		}
    +		Assert.assertArrayEquals(reducedArguments, prog.getArguments());
    +		Assert.assertEquals(TEST_JAR_MAIN_CLASS, prog.getMainClassName());
     	}
     
     	@Test
    -	public void testValidVariantWithNoJarAndNoArgumentsOption() {
    -		try {
    -			String[] arguments = {
    -					"--classpath", "file:///tmp/foo",
    -					"--classpath", "file:///tmp/bar",
    -					getTestJarPath(),
    -					"--debug", "true", "arg1", "arg2" };
    -			URL[] classpath = new URL[] { new URL("file:///tmp/foo"), new URL("file:///tmp/bar")
};
    -			String[] reducedArguments = {"--debug", "true", "arg1", "arg2"};
    -
    -			RunOptions options = CliFrontendParser.parseRunCommand(arguments);
    -			assertEquals(getTestJarPath(), options.getJarFilePath());
    -			assertArrayEquals(classpath, options.getClasspaths().toArray());
    -			assertArrayEquals(reducedArguments, options.getProgramArgs());
    -
    -			CliFrontend frontend = new CliFrontend(
    -				new Configuration(),
    -				Collections.singletonList(new DefaultCLI()),
    -				CliFrontendTestUtils.getConfigDir());
    +	public void testValidVariantWithNoJarAndNoArgumentsOption() throws Exception {
    +		String[] arguments = {
    +				"--classpath", "file:///tmp/foo",
    +				"--classpath", "file:///tmp/bar",
    +				getTestJarPath(),
    +				"--debug", "true", "arg1", "arg2" };
    +		URL[] classpath = new URL[] { new URL("file:///tmp/foo"), new URL("file:///tmp/bar")
};
    +		String[] reducedArguments = {"--debug", "true", "arg1", "arg2"};
    +
    +		RunOptions options = CliFrontendParser.parseRunCommand(arguments);
    +		assertEquals(getTestJarPath(), options.getJarFilePath());
    +		assertArrayEquals(classpath, options.getClasspaths().toArray());
    +		assertArrayEquals(reducedArguments, options.getProgramArgs());
    +
    +		Configuration configuration = new Configuration();
    +		CliFrontend frontend = new CliFrontend(
    +			configuration,
    +			Collections.singletonList(new DefaultCLI(configuration)));
     
    -			PackagedProgram prog = frontend.buildProgram(options);
    +		PackagedProgram prog = frontend.buildProgram(options);
     
    -			Assert.assertArrayEquals(reducedArguments, prog.getArguments());
    -			Assert.assertEquals(TEST_JAR_MAIN_CLASS, prog.getMainClassName());
    -		}
    -		catch (Exception e) {
    -			e.printStackTrace();
    -			fail(e.getMessage());
    -		}
    +		Assert.assertArrayEquals(reducedArguments, prog.getArguments());
    +		Assert.assertEquals(TEST_JAR_MAIN_CLASS, prog.getMainClassName());
     	}
     
     	@Test(expected = CliArgsException.class)
     	public void testNoJarNoArgumentsAtAll() throws Exception {
    +		Configuration configuration = new Configuration();
     		CliFrontend frontend = new CliFrontend(
    -			new Configuration(),
    -			Collections.singletonList(new DefaultCLI()),
    -			CliFrontendTestUtils.getConfigDir());
    +			configuration,
    +			Collections.singletonList(new DefaultCLI(configuration)));
     		assertTrue(frontend.run(new String[0]) != 0);
    --- End diff --
    
    Test does not make sense. `frontend.run` will throw an exception. Therefore `assertTrue`
has no effect on the outcome.


> Do not pass Configuration and configuration directory to CustomCommandLine methods
> ----------------------------------------------------------------------------------
>
>                 Key: FLINK-8340
>                 URL: https://issues.apache.org/jira/browse/FLINK-8340
>             Project: Flink
>          Issue Type: Sub-task
>          Components: Client
>    Affects Versions: 1.5.0
>            Reporter: Till Rohrmann
>            Assignee: Till Rohrmann
>              Labels: flip-6
>             Fix For: 1.5.0
>
>
> Currently, all methods in {{CustomCommandLine}} need a {{Configuration}} and sometimes
the configuration directory. Since these values should not change over the lifetime of the
{{CustomCommandLine}} we should pass them as a constructor argument instead of a method argument.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message