-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-18155. Refactor tests in TestFileUtil #4053
HADOOP-18155. Refactor tests in TestFileUtil #4053
Conversation
402ea51
to
b101100
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
b101100
to
78f613a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always good to see a bit of test cleanup.
Trying to keep all our tests modern what is a losing battle but
- move to LambdaTestUtils.intercept where you are updating try/catch (expected) clauses
- now you are adding validation of the File operations, maybe best to factor this out into a set of utility methods
void exists(File f) {
assertTrue("Not found " + f, f.exists())
}
that will give meaningful messages and reduce the amount of duplication.
production side tuning LGTM.
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
* Using LambdaTestUtils for asserting expected exceptions.
* Added TestFileUtil#assertDelListLength to assert that expected length of TestFileUtil#del
* Added TestFileUtil#createNewFileAndVerify that creates a new file and verifies if the operation was successful.
* TestFileUtil$Verify helps in performing File operations and also verify them.
@steveloughran I've addressed your comments. Could you please take another pass at this PR? |
This comment was marked as outdated.
This comment was marked as outdated.
85cf1dc
to
69da555
Compare
This comment was marked as outdated.
This comment was marked as outdated.
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
lovely
🎊 +1 overall
This message was automatically generated. |
all good, merge when you are happy, to whichever branches you want |
(cherry picked from commit d0fa9b5) Conflicts: hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
Description of PR
We need to ensure that we check the results of file operations whenever we invoke mkdir, deleteFile etc. and assert them right there before proceeding on. Also, we need to ensure that some of the relevant FileSystem APIs don't return null.
How was this patch tested?
Unit tests on CI.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?