-
Notifications
You must be signed in to change notification settings - Fork 0
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
#227 - temp dir is cleaned up #375
#227 - temp dir is cleaned up #375
Conversation
Codecov Report
@@ Coverage Diff @@
## master #375 +/- ##
============================================
- Coverage 89.21% 89.20% -0.01%
+ Complexity 292 291 -1
============================================
Files 48 48
Lines 1354 1353 -1
Branches 63 63
============================================
- Hits 1208 1207 -1
Misses 137 137
Partials 9 9
Continue to review full report at Codecov.
|
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.
@olenagerasimova thanks, please see one comment
@@ -132,6 +139,29 @@ void updatesDifferentReposSimultaneouslyTwice(final UpdateType update) throws Ex | |||
); | |||
} | |||
|
|||
@ParameterizedTest | |||
@EnumSource(UpdateType.class) | |||
@Order(Integer.MAX_VALUE) |
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.
@olenagerasimova could you please explain why order is important in this test suite? Usually it's a bad sign meaning tests are too coupled
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.
@olegmoz on each update Rpm
create two temp dirs with prefixed meta
and repo
, running this test after all other tests we can be sure, that not only temp dirs from this particular update were removed, but also all temp dirs from each update of this test class. I've caught the error with onTerminate
this way...
I understand, that this logic is not obvious and honestly I'm not a fun of this approach. Another solution I can think of here is to add these assertions to @After
method, but it does not seem to be a nice way either. What do you say?
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.
@olenagerasimova adding the checks to @After
method does not sound good. I guess proper way to do it is to add a check to every test case
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.
@olenagerasimova thanks, looks good to merge
Job |
Part of #227
Created test to verify that temp dir is cleaned up and fixed
Rpm
to callonTerminate
on the outer completable.