Conversation
dlstadther
left a comment
There was a problem hiding this comment.
If i'm not mistaken, this is the exact place where the previous PR attempt started and where I asked for test coverage where possible to confirm behavior and address the codecov failures.
Can you look into testing options?
|
Hi @dlstadther, It looks like there's a slight difference in that all existing unit tests are now passing (previously it seems that there were some possibly spurious failures). Currently the test coverage change is net zero; the only failure is that this file falls below targets. I did look a bit into adding a new test. It looks like there is insufficient mocking of the sch object, but I'm not sure how deep it needs to go. FWIW all the existing tests in this class are marked |
|
Luigi's codecov stuff has always been a pain. I'll take a look at it over the coming days and see what changes I can make to it to try reducing issues it causes without getting too relaxed with test coverage expectations. |
dlstadther
left a comment
There was a problem hiding this comment.
Sorry for the delay, but i pulled the codecov strictness back and now this PR is passing.
Taking a shot at #3351. I brought it up to latest master to see if it's still failing CI. The CI logs for #3351 are no longer available, so I wanted to re-run CI to see what's going on.