search

cognitive-engineering-lab / rust-book

The Rust Programming Language: Experimental Edition
https://rust-book.cs.brown.edu
Other
509 stars 83 forks source link

Add e2e tests for feedback feature #4

Closed connorff closed 1 year ago

connorff commented 1 year ago

This PR adds end-to-end tests for the feedback feature using jest-puppeteer.

I decided to move away from react-testing-library because it doesn't do rendering, thus our use of getBoundingClientRect won't work. As the issue suggests, we could mock this with fake data but, because the popper must be placed close to the highlight, it was easier and more realistic to use puppeteer.

Each test starts with a refreshed page so there is some code duplication across the its. If you prefer to just continue where the last it left off, I'd be happy to do that instead.

Also - after flailing around a bit with CI, I got it working but, the quiz preprocessor failed once - not sure if you had any ideas on why. Hopefully that doesn't mean they'll be flaky.

willcrichton commented 1 year ago

Is there a strong reason to use a headless renderer like puppeteer instead of something like jsdom? c.f. how the mdbook-quiz tests are setup: https://github.com/willcrichton/mdbook-quiz/tree/main/js/tests

connorff commented 1 year ago

Is there a strong reason to use a headless renderer like puppeteer instead of something like jsdom? c.f. how the mdbook-quiz tests are setup: https://github.com/willcrichton/mdbook-quiz/tree/main/js/tests

JSDom doesn't do any rendering, so calls like getBoundingClientRect will fail, causing the tooltip component to break without proper mocking. However, we probably don't want to mock something like getBoundingClientRect because the position relative to the highlighted text is important in determining how the tooltip works (ie. if the tooltip isn't properly attached to the text, when the user shifts their hover from the text to the tooltip, it will disappear).

JSDom also doesn't trigger the selectionchange event when highlighting text for some reason (but we can try manually firing it if need be).

I can work on a test implementation using JSDom if you'd prefer.

willcrichton commented 1 year ago

Oh sorry, I totally skipped over the comment at the top of the PR explaining this. I think it's fine for the reasons you explain.

I also saw the quiz preprocessor being flaky, but I think that's the validation script. I'll try to fix that.

willcrichton commented 1 year ago

I think this should fix flaky CI: https://github.com/willcrichton/rust-book/commit/f94e0d01aa9c98128683bbf87d8f9f18d46f116b

connorff commented 1 year ago

I think this should fix flaky CI: f94e0d0

Awesome - shouldn't conflict with my changes so I think this is good for another look.