-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Now tests with assumption violations are flagged with 'I' #1528
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for taking this on.
Could we hold off a bit on this? There were recent comments on the bug.
@@ -137,7 +137,7 @@ public void testIgnored(Description description) throws Exception { | |||
|
|||
@Override | |||
public void testAssumptionFailure(Failure failure) { | |||
// do nothing: same as passing (for 4.5; may change in 4.6) | |||
ignoreCount.getAndIncrement(); |
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.
In #1294 (currently targeted for 4.13) we are proposing updating this class to collect assumption failure counts, so I am not sure we should make this change.
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.
Oh great! in my opinion add a counter for the assumption failures would be great in order to know exactly how many did the test have
@@ -137,7 +137,7 @@ public void testIgnored(Description description) throws Exception { | |||
|
|||
@Override | |||
public void testAssumptionFailure(Failure failure) { | |||
ignoreCount.getAndIncrement(); | |||
// do nothing: same as passing (for 4.5; may change in 4.6) |
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.
This is absolutely wrong. We rely on it!
You cannot remove the line. There is no field counter for assumptions and even if it was there, it is a change in behavior in all other frameworks and their integration tests will fail.
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.
@Tibor17 I suggest commenting on the bug.
According to #1525 now the tests that has assumption violations are flagged with 'I' in the textUI output