Use !shouldfail rather than !mayfail on unit tests #2256
Conversation
| @@ -7,7 +7,7 @@ | |||
|
|
|||
| #ifdef SHARING | |||
|
|
|||
| SCENARIO("irept_sharing_trade_offs", "[!mayfail][core][utils][irept]") | |||
| SCENARIO("irept_sharing_trade_offs", "[!shouldfail][core][utils][irept]") | |||
There was a problem hiding this comment.
I'm not sure I agree with this one: it should actually not fail, it's just a currently known bug in the implementation... (the same is true of the other change in this file). I'd suggest @NathanJPhillips comment on this.
There was a problem hiding this comment.
(Nathan is AL atm, though this isn't critical so can wait if you don't agree with below)
I personally think if it starts not failing, we should turn it on (i.e. remove the flag). This current flag allows it to fluctuate between working and not working without anyone ever noticing. By changing it to !shouldfail, if someone fixes it, the tests will fail and they will know to turn them on (and not accidentally write a bunch more!)
There was a problem hiding this comment.
You might want to take a quick look at #1980 for the related discussion. I'm fine with your approach.
|
I'm happy with this change (after finding it difficult to figure out which test was actually failing on CI), but could it also be documented that |
|
The travis output still seems to be outputing failing unit tests inspite of docs so I'll need to look into why that as otherwise this serves no purpose 🙃 |
|
@thk123, why cannot we negate the assertion logic to make the tests actually pass (unless they are known bugs)? |
|
@peterschrammel Well I think they are known bugs, so when they're fixed we just want to turn them on (rather than having to flip all the failing conditions). |
|
I think the name "shouldfail" is unfortunate (I prefer "xfail", short for "expected to fail", which is what pytest uses). Still, I think the way that @thk123 is using it is good: in the long term no tests should be marked |
There was a problem hiding this comment.
Approved assuming it does what @thk123 originally thought it did in terms of reducing output and making it easier to see what is really failing.
|
@thk123 I cherry-picked this commit and I also see that it still prints the failed assertions for I still think that it is worth merging this PR so that if a bug gets inadvertently fixed we notice. |
|
@owen-jones-diffblue yup you're right, must have misread it. I agree this is still better so I shall rebase and merge it (to ensure no shouldfail unit tests have been fixed in the meantime) |
|
And I guess you should check there aren't any new mayfail tests. |
May fail runs the test and if it fails prints the output. Should fail runs the test and only prints the output if it actually passes. This makes it easier when looking at build output to determine which unit test has actually failed.
thk123
force-pushed
the
improvement/mark-failing-unit-tests-as-should-fail
branch
from
83d6c43 to
e93bf20
Compare
|
So JBMC unit tests are failing - I note that both the AWS build and the make builds don't fail which makes me think they are not running the JBMC unit tests. @peterschrammel are you aware of this? |
|
@thk123 @peterschrammel Has the AWS / make build / unit test issue been resolved? |
|
Replaced by #5248 |
May fail runs the test and if it fails prints the output. Should fail runs the test and only prints the output if it actually passes. This makes it easier when looking at build output to determine which unit test has actually failed.