HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster #1316
Conversation
Change-Id: I67ee849cd674b0c3541416bb1f78869541313def
There was a problem hiding this comment.
Thanks @smengcl for this speedup. Can you please add a comment that new tests should be added as private methods invoked from testFileSystem() to help avoid regression next time?
Sure thing. I've added a note in the javadoc of the test class. |
| @@ -154,21 +155,21 @@ public void testCreateFileShouldCheckExistenceOfDirWithSameName() | |||
| } catch (FileAlreadyExistsException fae) { | |||
| // ignore as its expected | |||
| } | |||
|
|
|||
| // Cleanup | |||
| fs.delete(new Path("/d1/"), true); | |||
There was a problem hiding this comment.
Shouldn't the cleanup be in a finally block? post mkdir if the test fails the directory won't get cleaned up. Wouldn't that affect other tests?
There was a problem hiding this comment.
Thanks for taking a look at this @ayushtkn .
In this case it doesn't matter IMO. Cause if this test (testCreateFileShouldCheckExistenceOfDirWithSameName) fails and throws, testFileSystem as a whole will fail at this point thus the tests following this one won't even be executed.
There was a problem hiding this comment.
hmm, correct the following tests will fail. Do you think, thats a good behaviour, Ideally only the affected test should fail. If there is a common cluster, that could have been initialised in a BeforeClass and all other tests could have ran independently.
Anyway not related to what you are doing here. Thanx for confirming.
There was a problem hiding this comment.
Yes, I also think @BeforeClass is better. Maybe another jira to refactor the test class this way.
There was a problem hiding this comment.
There was a problem hiding this comment.
LGTM.
Initialising cluster can be though done in BeforeClass and the tests can be made independent, preventing failure of one affecting subsequent tests. May be sometime in future? if there were so specific reasons for doing it this way.
Anyway that isn't something being targeted here. just wanted to share my thoughts on this. :-)
|
Thanks @ayushtkn for reviewing this. I have filed https://issues.apache.org/jira/browse/HDDS-4162 for the follow-up refactoring (use Will merge shortly. |
* master: (26 commits) HDDS-4167. Acceptance test logs missing if fails during cluster startup (apache#1366) HDDS-4121. Implement OmMetadataMangerImpl#getExpiredOpenKeys. (apache#1351) HDDS-3867. Extend the chunkinfo tool to display information from all nodes in the pipeline. (apache#1154) HDDS-4077. Incomplete OzoneFileSystem statistics (apache#1329) HDDS-3903. OzoneRpcClient support batch rename keys. (apache#1150) HDDS-4151. Skip the inputstream while offset larger than zero in s3g (apache#1354) HDDS-4147. Add OFS to FileSystem META-INF (apache#1352) HDDS-4137. Turn on the verbose mode of safe mode check on testlib (apache#1343) HDDS-4146. Show the ScmId and ClusterId in the scm web ui. (apache#1350) HDDS-4145. Bump version to 1.1.0-SNAPSHOT on master (apache#1349) HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster (apache#1316) HDDS-4149. Implement OzoneFileStatus#toString (apache#1356) HDDS-4153. Increase default timeout in kubernetes tests (apache#1357) HDDS-2411. add a datanode chunk validator fo datanode chunk generator (apache#1312) HDDS-4140. Auto-close /pending pull requests after 21 days of inactivity (apache#1344) HDDS-4152. Archive container logs for kubernetes check (apache#1355) HDDS-4056. Convert OzoneAdmin to pluggable model (apache#1285) HDDS-3972. Add option to limit number of items displaying through ldb tool. (apache#1206) HDDS-4068. Client should not retry same OM on network connection failure (apache#1324) HDDS-4062. Non rack aware pipelines should not be created if multiple racks are alive. (apache#1291) ...
What changes were proposed in this pull request?
4 new tests have been added since HDDS-2833 and are not sharing that MiniOzoneCluster.
I am able to cut down the run time of TestOzoneFileSystem from 3m18s to 1m2s on my Mac. It would only save more run time on GitHub Workflow.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4109
How was this patch tested?
No new tests needed.