Skip to content

[hotfix]Harden TextInputFormatTest to avoid failure by uncleaned files#8304

Merged
zentol merged 2 commits intoapache:masterfrom
Aitozi:test_fix
May 16, 2019
Merged

[hotfix]Harden TextInputFormatTest to avoid failure by uncleaned files#8304
zentol merged 2 commits intoapache:masterfrom
Aitozi:test_fix

Conversation

@Aitozi
Copy link
Contributor

@Aitozi Aitozi commented Apr 28, 2019

What is the purpose of the change

When I run test locally, it happened to failed and with the uncleaned temp file leaving in the tmp dir. So the test case can not pass ever until i delete the correspond files.

It use deleteOnExit to clean up the temp file created by test case, But the method may not work when JVM crash or killed. I think it may make the test case fragile.

Brief change log

Add the @before and @after prepare / cleanup method

@flinkbot
Copy link
Collaborator

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@Aitozi
Copy link
Contributor Author

Aitozi commented Apr 28, 2019

And I see there are quite a lot other tests rely on deleteOnExit to do clean up. Should I do the fix for them, too? Please take a look when you free , Thanks @zentol .

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cleanup isn't really the problem; you cannot ensure 100% that files are deleted. The real issue here is testNestedFileRead using constant paths.

private final String tempDirName = "tempDir";

@Before
public void setUp() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a TemporaryFolder instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems better, I will change it then. But a little question, why my approach can not ensure the file cleaned (I check and clean the related file before execute and after test method), Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the JVM crashes before the delete calls are executed the files will remain.

@zentol zentol self-assigned this Apr 29, 2019
@Aitozi
Copy link
Contributor Author

Aitozi commented May 10, 2019

I update the PR, please take a look when you are free, thanks @zentol

@zentol zentol merged commit 6613332 into apache:master May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants