react-flip-move
react-flip-move copied to clipboard
joshwcomeau
Tests refactoring checklist
Initially, I created a project, but then I realised that it's not commentable so maybe an umbrella issue with checklist would be more useful, so I duplicate it as such.
@joshwcomeau has mentioned that he would like to have some improvements. So I decided to make some kind of checklist for what has to be done. Feel free to add anything that you think would be nice to do.
Definitely needed:
- [x] Isolate the tests from each other, i.e. make all the setup before each test so that the state is reset #166
- [ ] Add tests for appear / enter / leave animations
- [x] Do something with tests stability (see false positives like that: https://travis-ci.org/joshwcomeau/react-flip-move/jobs/236306660) #175
- [x] Add tests for runtime props conversion and checks #174
- [ ] Use sandboxed sinon for easier resetting/restoring
Maybe needed:
- [x] ~~Add simple unit tests for helpers~~ — not really needed, they are fully covered by other tests
- [x] Use some code coverage tool like istanbul #172
- [x] Use enzyme for easier react output manipulation #169
- [x] ~~Use sinon fake timers instead of
setTimeout~~
Awesome!
Yeah, so I originally wrote this module when I was completely inexperienced with React tests. I still don't have a ton of experience with it, but I can certainly see how bad they are now.
The easiest and most crucial win is just separating each test, as you say, so that each test mounts its own DOM, performs the test, and then unmounts it.
The false-positives thing has been very tricky in my experience - Working locally, the timing is very accurate, but on CI the timing is far less predictable. An animation set to 500ms will usually be within a few ms locally, but can take 1000-1500ms on travis, for reasons that aren't super clear to me.
The "maybe needed" stuff all sounds good to me - the helpers really should be unit-tested, and I like Istanbul/Enzyme.
I've added one more point: "use sinon fake timers instead of setTimeout". To do that, we also need to use some stub for requestAnimationFrame like this one, or write one ourselves as react-motion did.
Upd: OK, it's not really achievable as we're using CSS transitions
The false-positives thing has been very tricky in my experience - Working locally, the timing is very accurate, but on CI the timing is far less predictable. An animation set to 500ms will usually be within a few ms locally, but can take 1000-1500ms on travis, for reasons that aren't super clear to me.
Maybe the problem is that we don't take two skipped frames into account?
Maybe the problem is that we don't take two skipped frames into account?
Yeah, maybe if CI only runs at 1-2fps, it could explain it.
-
regarding " Use sandboxed sinon for easier resetting/restoring", have you given jests mock functionality a look? we've replaced also most all of our sandboxes with the mocking toolset provided by jest.
-
regarding "Add tests for appear / enter / leave animations" we could look for classes mapped to animation ("--anim-start" etc.) maybe? i mean, short of that you need something like cypress.io for actual e2e testing i wager.
-
adding tests for high pressure situations (500 incoming elements, 200 leaving, 600incoming all incoming in short distance). it'll most likely mess up sorting. we'd then have a red to work up from. i'll probably build something in that regard this weekend
