Fix notebook tests #675
Fix notebook tests #675
Conversation
|
Notebook CI run: |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #675 +/- ##
=======================================
Coverage 79.12% 79.12%
=======================================
Files 126 126
Lines 8895 8895
=======================================
Hits 7038 7038
Misses 1857 1857
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -11,13 +11,8 @@ addopts = | |||
| --tb native | |||
| -W ignore | |||
| --cov=alibi_detect | |||
| --randomly-dont-reorganize | |||
There was a problem hiding this comment.
Why do we remove this here?
There was a problem hiding this comment.
For --randomly-seed=0, I've moved it to ci.yml because having it in the global pytest config caused an error in the notebook CI because pytest-randomly is now disabled there.
For --randomly-dont-reorganize, I decided we could remove it anyway. This controls whether or not the order of tests is randomly reorganized. From the pytest-randomly docs:
By randomly ordering the tests, the risk of surprising inter-test dependencies is reduced - a technique used in many places, for example Google’s C++ test runner googletest. Research suggests that “dependent tests do exist in practice” and a random order of test executions can effectively detect such dependencies [1].
There was a problem hiding this comment.
btw we can add --randomly-dont-reorganize to ci.yml if you think this change shouldn't be made without further discussion...
There was a problem hiding this comment.
Ah yeah, I was curious about --randomly-dont-reorganise, it seems useful no? I suppose because we don't really deal with persistent state it's not the type of thing that's going to help us much. Although perhaps it's worth keeping in for the saving tests, that's the only place where I can image one test leaking into another...
There was a problem hiding this comment.
Yeh agree that I can't think of any obvious places where tests might be leaky. I hope that the saving ones are ok since they use pytest's tmp_path. I sort of think if we keep pytest_randomly it doesn't hurt to randomise test order, but if we're removing it its not worth implementing an alternative solution to randomise tests.
There was a problem hiding this comment.
Removing --randomly-dont-reorganise actually enables random order, right? In which case it seems useful to have?
There was a problem hiding this comment.
Yep, random order has been enabled by removing the flag.
There was a problem hiding this comment.
@jklaise just to confirm, are you saying random order seems useful to have? i.e. we stick to having the flag removed...
There was a problem hiding this comment.
OK thanks. Think this is good to merge then 👍🏻
|
@jklaise @mauicv I'm now wondering whether we park this until we've decided what to do about However, #664 uncovered a problematic issue with the way custom |
|
@ascillitoe I think it would be great if we could get the notebook tests passing again, I don't think it's a big deal to disable |
pytest-randomlyis disabled in the notebook CI in order to resolve #664 (see pytest-dev/pytest-randomly#495). This isn't an ideal long-term solution, we might want to think about replacingpytest-randomlyif pytest-dev/pytest-randomly#495 is not going to be fixed.