implement approval conditioned on issue severities #141
Conversation
t-8ch
force-pushed
the
feature/approval-severity
branch
from
ebc6931
to
546797a
Compare
t-8ch
force-pushed
the
feature/approval-severity
branch
from
546797a
to
ab2cd9e
Compare
|
Oops, I have updated my PR too |
| StashIssueReportingPostJob.shouldApprovePullRequest(Optional.empty(), report) | ||
| ); | ||
|
|
||
| report.clear(); |
There was a problem hiding this comment.
multiple asserts in a single test method are considered a test smell
It is often better to split such a test method into separate cases. One can give those separate test method meathingful names and do a proper setup in those smaller methods
There was a problem hiding this comment.
In general I agree with this being a smell.
However as far as I know this is a concern when testing objects with (complex) internal state.
So when doing multiple asserts the later ones may depend on implicitly changed state in the object.
As the tested functionality here is only a single static method there is no state.
Furthermore by doing it this way if some assertion failed is immediately obvious how this assertion differed from the previous one.
If there are multiple test functions all doing essentially the same setup (only progressing) then I first have to compare all the test methods where the actual differences are.
There was a problem hiding this comment.
The test with multiple asserts might fail at first assertion and other assertions are not executed at all.
It is good when one break makes only one assert fail, letting other asserts be executed in separate test methods.
As soon as you break test code to separate tests, you have a problem of repetitous setup. It is solved easily by single @Before method or a helper method called in each method.
If the assert should be checked only after some other checks have succeeded (which are checked in separate test methods) then one can use assume* methods of JUnit.
As soon as you have your tests split this way, single defect breaks one test only. Some other tests may become yellow telling you the defect is essential for checks performed there. The good thing you'd know what have failed exactly. If several things have failed you'd know all of them.
There was a problem hiding this comment.
I totally agree with you if the object under test is complex and contains internal state. However for a static method I think it is overkill. I very much prefer a simple test execution where I can see the difference of the parameters between the assertions very easily.
It would be even better to have nice parameterized tests in Java like in python, but in Junit this involves a ridiculous amount of boilerplate hiding the actual logic.
Can we agree to disagree on this one with me (ab)using my maintainer powers to keep the status quo?
|
|
||
| report.add(minorIssue); | ||
| assertTrue( | ||
| StashIssueReportingPostJob.shouldApprovePullRequest(Optional.of(Severity.MINOR), report) |
There was a problem hiding this comment.
As far as I can see, shouldApprovePullRequest is an internal method, that is implementation details.
You might test it until one wants to change internal implementation details. Normally tests should not be modified upon such a change to ensure public contract is not changed. However when internal methods are exposed and tested, tests must be modified too. It can lead to extra work
There was a problem hiding this comment.
The method is made a package-scoped static method especially to improve testing.
There is no need for any fixtures or the mocking of dependencies.
|
lgtm |
Supersedes #140