gnomad-browser icon indicating copy to clipboard operation
gnomad-browser copied to clipboard

Add .md transformer and static page tests

Open rileyhgrant opened this issue 2 years ago • 4 comments

Implements a simple .md transformer that renders the raw stringified .md content as the __html attribute in a react element, for the purposes of snapshot testing.

Includes includes a .pdf transformer that uses the name of the source .pdf file

Includes snapshot tests for the static pages:

  • Home
  • About
  • Team
  • Downloads
  • Policies
  • Publications
  • Feedback
  • Help

These represent the mostly static pages that are unlikely to change. Snapshot tests for these unchanging pages will help in catching unintentional regressions going forward.

rileyhgrant avatar Sep 21 '22 20:09 rileyhgrant

While this transformer doesn't render the .md content exactly as it would be on the browser (it doesn't get passed through the markdownLoader), I'd argue it still represents a very useful set of tests.

This seems like the correct division of responsibility to me. Testing takes place in the relevant repository, and as such this repo does not test some elses code (though in this case someone else is us, but in a different repo).

Candidly, implementing an Async Transformer with Jests docs was proving very difficult for me. If there is some simple thing I was missing I would be excited at this point, but after trying many things and scouring many stack overflow and github threads, I believe the juice of passing the .md content through the markdownLoader in snapshot tests is not worth the squeeze, and that testing the raw .md content is almost an identical test.

Thoughts / opinions?

rileyhgrant avatar Sep 21 '22 20:09 rileyhgrant

Edit: inline snapshots are no longer being used, and the pre-commit hook for trailing whitespace is not removed.


The Jest testing function toMatchInlineSnapshot() seems like a good fit for many tests that don't rely on tests that use some variation of a loop syntax (they don't support multiple inline snapshots for one looped line of code). However, the pre-commit hook trailing-whitespace won't accept the string literals the Jest function outputs, as it seems to output whitespace on lines as part of the inline snapshots.

I removed the trailing whitespace pre-commit hook as part of this PR, but there may be a better way to do this, and this possibly warrants discussion. It does seem that prettier already removes trailing whitespace as part of its reformatting, with a bit more indiscretion than the pre-commit hook does.

rileyhgrant avatar Sep 22 '22 22:09 rileyhgrant

Also - there seem to be some strange configuration differences happening, Phil and I were unable to replicate the same error on our local machines on a Zoom call when pulling from this topic branch.

I have re-created a snapshot for the DatasetSelector.spec.tsx test, as my machine was mirroring the errors that CI was outputting. Doing so is causing CI to succeed now, however there still seems to be possibly something weird going on, and may warrant discussion in the near future. I opted to replace the snapshot as both CI and my machine were in agreement with what errors were being output.

rileyhgrant avatar Sep 22 '22 22:09 rileyhgrant

Re: discussion Phil and I had about differing newline characters possibly causing snapshot test issues, I have checked and both the previous and new snapshot for DatasetSelector.spec.tsx, one that was causing particular issues, used LF as the newline character, per

git ls-files --eol

Still stumped on what actually changed, and what is causing the different development environments on CI, my machine, and Phil's machine.

rileyhgrant avatar Sep 23 '22 13:09 rileyhgrant

The Jest testing function toMatchInlineSnapshot() seems like a good fit for many tests that don't rely on tests that use some variation of a loop ...

Re: a discussion with Phil some time back, this PR has moved away from doing inline snapshots, and kept the pre-commit hooks intact. Writing a comment here to keep a logical flow in the thread, and also added in edit to the initial message just so it's clear that that is no longer the case.


This PR also includes the resulting updated snapshots after running yarn jest with the -u flag on my machine. Without doing this, the CI was failing for certain snapshot tests.

rileyhgrant avatar Nov 22 '22 20:11 rileyhgrant