-
Notifications
You must be signed in to change notification settings - Fork 392
Conversation
54b40b7
to
18c0da8
Compare
18c0da8
to
c1dabef
Compare
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.
Major kudos for taking this on 🙌 Bringing old and unwieldy test suites up to date is pretty thankless work, and I really appreciate it. It's looking much better and I see it'll fix a bunch of longstanding flakes along the way.
How are we doing on test coverage so far? This is one of the suites that didn't have great test coverage in the past (and that's even more of a pain to bring up to date after the fact 😅 ). You can check it out locally by running npm run test:coverage
and loading the .lcov
file with a package like atom-lcov.
I see you've still got the last commented-out bit left to do - re-request review from me when you're ready and we'll get this in 😄
Co-Authored-By: Ash Wilson <smashwilson@github.com>
It doesn't seem super awful. I have not covered any new branches though, as I was just updating the old code.
Also, the failed test case was not me it was a flake. |
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.
✨
Description of the Change
Benefits
Possible Drawbacks
This will slow down test, because we were previously not preventing state leak.
Applicable Issues
#2308 Ran into trouble implementing good test cases, because state leak was corrupting the results.
Metrics
Tests
I fixed 'em 😉
Release Notes
Updated test case accuracy.