[FLINK-6397] MultipleProgramsTestBase does not reset ContextEnvironment #3810

ifndef-SleePy · 2017-05-02T15:34:39Z

Reset ContextEnvironment when finished testing in MultipleProgramsTestBase.java and some other ITCases.
Remove some useless importing.
Add unsetContext method in TestEnvironment.java.

zentol · 2017-05-03T08:59:19Z

...rent/flink-test-utils/src/main/java/org/apache/flink/test/util/MultipleProgramsTestBase.java

 		switch(mode){
 			case CLUSTER:
-				new TestEnvironment(cluster, 4).setAsContext();
+				testEnvironment = new TestEnvironment(cluster, 4);


I would suggest moving this into an @Before method and calling unsetAsContext in a @After method.

Sounds reasonable.

@zentol I just found that I can not move these codes into @before method, because it will not work with JUnit Parameterized. I proposal to keep these codes in constructor. What do you think?

Could you try this instead: junit-team/junit4#45 (comment)

Thank you for responding. Last my comment is not correct. Actually what I want to say is that we can not move these code to @BeforeClass and @afterclass. But we can move them to @before and @after, however it will bring a little overhead since @before and @after will be called for each test method. I pushed a new commit using @before and @after.

StephanEwen · 2017-05-03T11:35:58Z

I would suggest to use for TestEnvionment the same approach as in TestStreamEnvironment (a static unset method). Then you don't need to ever remember a reference to the environment, which makes the changes much smaller.

ifndef-SleePy · 2017-05-04T02:55:24Z

@StephanEwen
I agree that a static unset method would be a much easier implementation. Do you think it's acceptable that the unset method is static but set method is not static in TestEnvironment? Or we can implement the set method as static too, but that will make more changes.

ifndef-SleePy · 2017-05-07T03:08:17Z

I think Till has implemented a static unsetAsContext method in TestEnvironment recently. I rebased the implementation from master, and recommit the pull request.

zentol

Bar some minor comments this looks good to merge.

zentol · 2017-05-08T15:20:47Z

...ent/flink-test-utils/src/main/java/org/apache/flink/test/util/CollectionTestEnvironment.java

@@ -49,7 +49,7 @@ public JobExecutionResult execute(String jobName) throws Exception {
 		return result;
 	}

-	protected void setAsContext() {
+	public void setAsContext() {


do we still need this change?

zentol · 2017-05-08T15:21:29Z

...rent/flink-test-utils/src/main/java/org/apache/flink/test/util/MultipleProgramsTestBase.java

@@ -97,6 +105,12 @@ public MultipleProgramsTestBase(TestExecutionMode mode) {
 		}
 	}

+	@After
+	public void teardownEnvironment() {
+		TestEnvironment.unsetAsContext();


you could add another switch here, instead of calling both methods every time.

ifndef-SleePy · 2017-05-09T02:23:20Z

Hi, I updated this review. I don't know why there is no reminding in jira and email.

zentol · 2017-05-09T07:50:45Z

We don't get notifications about pushed commits in general, only for comments and created/closed pull requests.

ifndef-SleePy · 2017-05-10T09:36:09Z

OK, got it. Thank you for explanation. Is there any more problems with this pull request?

zentol · 2017-05-10T09:59:56Z

Not quite; the CassandraConnectorITCase was broken in the process. This test contains both batch and streaming jobs that rely on different TestEnvironments. When the batch TestEnvironment is set as the context the streaming tests will fail.

StreamExecutionEnvironment#getExecutionEnvironment() doesn't account for this case, and tries to create a LocalStreamEnvironment which however fails since no explicit environment may be create when a context environment was set.

This is more or less a problem of the cassandra test; it shouldn't actually run any jobs at all. Let me fix that, then we'll get back to this PR.

Besides that i don't see any problems right now.

zentol · 2017-05-12T21:51:03Z

I've fixed the cassandra tests, would be great if you could rebase this PR. You should be able to discard all changes to the cassandra tests in this PR.

…before and after method.

…e.java

ifndef-SleePy · 2017-05-13T03:38:09Z

Nice work!
I have rebased master.

zentol · 2017-05-13T15:47:32Z

merging.

This closes apache#3810.

zentol reviewed May 3, 2017

View reviewed changes

ifndef-SleePy changed the title ~~[Flink-6397] MultipleProgramsTestBase does not reset ContextEnvironment~~ [FLINK-6397] MultipleProgramsTestBase does not reset ContextEnvironment May 4, 2017

ifndef-SleePy force-pushed the FLINK-6397 branch from d0becf4 to ddf75b8 Compare May 7, 2017 02:49

zentol reviewed May 8, 2017

View reviewed changes

zentol mentioned this pull request May 10, 2017

[FLINK-5101] Refactor CassandraConnectorITCase #2866

Closed

biao.liub added 6 commits May 13, 2017 11:34

[FLINK-6397] Unset context when integration test is finished.

cf582a4

[FLINK-6397] Fix test cases.

126afe9

[FLINK-6397] Resolve some conflicts when rebase master.

e3f9b4c

[FLINK-6397] Move environment setup and teardown from constructor to …

2546618

…before and after method.

[FLINK-6397] Optimize control access in CollectionTestEnvironment.java.

486cabe

[FLINK-6397] Optimize teardown environment in MultipleProgramsTestBas…

9b0a927

…e.java

ifndef-SleePy force-pushed the FLINK-6397 branch from 62f98aa to 9b0a927 Compare May 13, 2017 03:35

asfgit closed this in 707f25f May 13, 2017

fanyon pushed a commit to fanyon/flink that referenced this pull request May 15, 2017

[FLINK-6397] [tests] Reset context environments after test execution

3bbca77

This closes apache#3810.

rmetzger added the component=Tests label Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-6397] MultipleProgramsTestBase does not reset ContextEnvironment #3810

[FLINK-6397] MultipleProgramsTestBase does not reset ContextEnvironment #3810

ifndef-SleePy commented May 2, 2017 •

edited

Loading

zentol May 3, 2017

ifndef-SleePy May 4, 2017

ifndef-SleePy May 7, 2017

zentol May 7, 2017

ifndef-SleePy May 8, 2017

StephanEwen commented May 3, 2017

ifndef-SleePy commented May 4, 2017

ifndef-SleePy commented May 7, 2017

zentol left a comment

zentol May 8, 2017

zentol May 8, 2017

ifndef-SleePy commented May 9, 2017

zentol commented May 9, 2017

ifndef-SleePy commented May 10, 2017

zentol commented May 10, 2017 •

edited

Loading

zentol commented May 12, 2017

ifndef-SleePy commented May 13, 2017

zentol commented May 13, 2017

[FLINK-6397] MultipleProgramsTestBase does not reset ContextEnvironment #3810

[FLINK-6397] MultipleProgramsTestBase does not reset ContextEnvironment #3810

Conversation

ifndef-SleePy commented May 2, 2017 • edited Loading

zentol May 3, 2017

Choose a reason for hiding this comment

ifndef-SleePy May 4, 2017

Choose a reason for hiding this comment

ifndef-SleePy May 7, 2017

Choose a reason for hiding this comment

zentol May 7, 2017

Choose a reason for hiding this comment

ifndef-SleePy May 8, 2017

Choose a reason for hiding this comment

StephanEwen commented May 3, 2017

ifndef-SleePy commented May 4, 2017

ifndef-SleePy commented May 7, 2017

zentol left a comment

Choose a reason for hiding this comment

zentol May 8, 2017

Choose a reason for hiding this comment

zentol May 8, 2017

Choose a reason for hiding this comment

ifndef-SleePy commented May 9, 2017

zentol commented May 9, 2017

ifndef-SleePy commented May 10, 2017

zentol commented May 10, 2017 • edited Loading

zentol commented May 12, 2017

ifndef-SleePy commented May 13, 2017

zentol commented May 13, 2017

ifndef-SleePy commented May 2, 2017 •

edited

Loading

zentol commented May 10, 2017 •

edited

Loading