just-the-docs / just-the-docs

A modern, high customizable, responsive Jekyll theme for documentation with built-in search.
https://just-the-docs.com
MIT License
7.46k stars 3.66k forks source link

Add regression tests for the theme #805

Open pdmosses opened 2 years ago

pdmosses commented 2 years ago

*Is your feature request related to a problem? Please describe.** When developing a PR for this theme, it can be difficult and/or tedious to check for backwards compatibility with the latest release.

Describe the solution you'd like I'd like to be able to specify and automatically run regression tests that check whether specified Markdown code produces specified HTML in specified configurations.

Describe alternatives you've considered A more tedious approach would be to include the Markdown code to be tested in the sources of a website that uses this theme, and use Jekyll to build the site for each configuration to be tested. The HTML produced by the latest release of the theme could then be compared with the HTML produced by the PR being developed.

I've experimented with including pages with such tests in a PR branch of the theme. But doing that in a release of the theme would have the disadvantage that adding or changing regression tests requires new releases...

Additional context I've set up a repository with some pages intended as regression tests for the v0.3.3 release of the theme. Apart from tests of standard Markdown features, it shows how to format math using the theme, and demonstrates how to use Jekyll-4 for publishing a site on GH Pages. A cleaned-up version of the repository could be transferred to the just-the-docs organization, and augmented with systematic tests of other Markdown features (e.g., those tested by the theme's Markdown kitchen sink, and by the tests collection of PR #578; see also the site published at https://pdmosses.github.io/just-the-docs/).

mattxwang commented 2 years ago

I think this is a good idea! One suggested approach is:

  1. Create some snapshot tests files with the "correct" Markdown-rendered HTML
  2. Build the site with bundle exec jekyll build
  3. Diff the generated HTML, sans-whitespace - I'm sure we could use diff, some Ruby utility, or some Node utility
  4. Have CI error if the diff fails

What are our thoughts on this? Perhaps there's a Ruby-esque way to do this that I've missed?

pdmosses commented 2 years ago

It seems the Jekyll project uses Ruby to specify automated checks, see https://github.com/jekyll/jekyll/tree/master/test. Many of the check files are associated with PRs.

I used diff when testing the bug fix implemented by PR #494. I wrote:

With the v0.3.3 regression tests active, diff -r -q on _site/docs reports that only _site/docs/tests/navigation/disambiguation/dca/index.html differs, which is due to the breadcrumb correction there.

However, I needed to check manually that the difference reported by diff corresponded to the fix implemented by the PR; I also browsed the affected page to check that the changed links were correct . I don't know how easy it would be to specify such checks in Ruby.

pdmosses commented 2 years ago

@mattxwang I've created just-the-docs/just-the-docs-tests, and I'll now proceed to add all my previous regression tests to it.

The home page shows how I created a simple Jekyll website that builds both locally and on GH Pages. I might develop a corresponding PR for the Just the Docs home page in time for v0.4.0 (although we can improve our theme docs pages at any time, independently of theme releases). Let me know[^1] if you notice any potential confusion in the explanations, or suboptimal code in the files. The explanations target new users of Jekyll themes; experienced users are likely to find them superfluous.

[^1]: I suggest to submit problems and suggestions for improvement as issues in just-the-docs/just-the-docs-tests, rather than discussing them in the present issue.

mattxwang commented 2 years ago

Thanks @pdmosses, appreciate you taking point on this (I thought I'd have more time to give it a spin, but unfortunately I didn't).

I might develop a corresponding PR for the Just the Docs home page in time for v0.4.0 (although we can improve our theme docs pages at any time, independently of theme releases).

Thanks! I don't have any strong comments on a brief glance, though I do think the bundle log is brittle. I can certainly leave more feedback in the repo and/or work with you on this, hopefully after publishing v0.4.0.

pdmosses commented 2 years ago

I do think the bundle log is brittle

Right; I think that I've now fixed that.

pdmosses commented 1 year ago

Following up to a comment in a PR by @mattxwang:

Should just-the-docs-tests use Jekyll 3 or Jekyll 4? If diff confirms that there are no significant differences in the built sites, I guess I won't need to check both versions…

Hm, that's a trickier question. Both is not an option, right?

Right, it's a lot of boring work to check all the current regression tests. And in theory, they should all be re-checked before merging each PR…

I'll soon add the last tests for RC4, but I'm not planning to re-check all the previous tests. I'll remove all the ✅ marks, as they mostly indicate tests that have been checked only for RC3.

If not, I would prefer to lean towards the current version (3) so that we can also test for our github-pages users. I do see the potential discrepancy that leads with our docs site; however, maybe this also gives us more "complete" coverage?

Right. In any case, it's easy enough to check specific tests with both versions of Jekyll if we suspect a bug due to a Jekyll difference.

As an aside - how much of the work is manual?

Currently, all of it…

I wonder how much of it we could automate with an action (we could listen on each commit, and output a diff as a comment on a PR for example).

There are also tools that require a bit of setup but resolve the visual use-case: Storybook is one that I used at CZI that's great for testing minute effects (even shifting a pixel) on both the look and functionality of entire sites.

Interesting – but presumably requiring a significant investment of time and effort.

I think the main benefit of the current tests is as documentation of corner cases and sanity tests. I originally hoped that it would be feasible to re-check them all manually for each PR(!) but that is clearly unrealistic, now there are so many of them.

pdmosses commented 1 year ago

The following proposal aims to integrate regression tests in the process of submitting and reviewing PRs. It also aims to distribute the work of maintaining the just-the-docs-tests website.

Suggestions for improvements or clarifications of the proposal are welcome, as are proposals for alternative ways of managing the development of regression tests for Just the Docs PRs.

Proposal

For each new PR that changes the theme code (i.e., other than chores and docs):

  1. The PR description should include easily-checkable black-box tests of the new/enhanced feature or bug fix;
  2. The reviewer(s) of the PR should review the tests for adequacy (clarity and rigour), and check them using the PR branch (e.g., as a remote theme);
  3. Shortly after the PR has been approved and merged, a reviewer should submit a PR to just-the-docs-tests to: a. add the checked tests to the appropriate collection in just-the-docs-tests; b. add the relevant line from the theme raw CHANGELOG to the list of checked tests; c. add a link definition for the PR title in the list of checked tests and in _includes/tests.md; and d. form a [short-cut link reference] from the PR title, author, and number.
  4. Finally, the author or a reviewer should add a deep link from the PR conversation to its checked tests.

For the PRs that have already been merged in preparation for v0.4.0, I've (1) formulated and (2) checked some tests myself. However, the tests have not yet been reviewed, and many of them could surely be improved.

So far, I've also done (3) myself. That wasn't as much work as it might seem: the use of short-cut link references minimises the editing of the line copied from the theme raw CHANGELOG.

Checking the tests has been useful in catching potential regressions before merging some PRs. The tests for new/enhanced features may also serve as examples of how to use those features, supplementing the explanations in the theme docs.

The links from the list of checked tests to the tests and PRs are active in the just-the-docs-tests repo as well as in the just-the-docs-tests website. This makes it easy to revisit the tests for specific PRs.

mattxwang commented 1 year ago

Thank you for elucidating these thoughts @pdmosses! I personally think that this is a welcome change. To add on, we can better encode some of these items of the proposal in:

One concern is that the size of the test repository would balloon very quickly as we add more features - some require combinations of features to adequately test (ex dark mode + enabling a certain site config + having certain pages). I don't think this is a blocker for this idea; but, a way to make this better would be nice!

Another thought is that some items seem somewhat hard to test with just-the-docs-tests - #1021 comes to mind. Also not a blocker per see, but something I wanted to call out.

Bigger picture (but requiring much more effort), I think it'd really be worthwhile to look into Storybook + Chromatic (or a similar alternative automated E2E visual unit test service) to make this much simpler for reviewers. I can't believe the amount of work you've already put in to just-the-docs-tests!


Given that I do a lot of theme development (and thus others end up being the ones reviewing my code), I'd like to also ping @just-the-docs/reviewers and see if any other maintainers/reviewers have any thoughts on this.