Site-kit-wp: Scaffold foundation for JS unit tests with Jest

[aaemnnosttv]

Introduced as part of the infrastructure for E2E tests in #157, Jest has been chosen as the test framework to use for JS unit tests going forward.

Work has already started to migrate the current QUnit tests to Jest in #437.

This is a foundational precursor to more substantial feature development and refactoring in the near future.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• The plugin should contain necessary configuration to run Jest unit tests.
• Travis-CI should run Jest unit tests present, and they should make the build fail as expected when they fail (highlighting because there were issues with the way QUnit was used at some point before).
• Any test directories should be excluded from the plugin production build (e.g. npm run release-zip).
• Optional: Migration of QUnit tests is generally not a requirement for this issue because of the work it entails. However, because of much (all?) of it already being done in #437, we can continue working towards the goal of eliminating QUnit.
• QUnit tests should continue to be run in Travis-CI unless actually all QUnit tests have been migrated to using Jest. If so, the entire QUnit infrastructure and references should be removed from the codebase.

Note regarding IB / estimate: How much additional work would it be to migrate all QUnit tests to using Jest, given the work already done? Would be great if you could provide estimates based on both. If too much different, the focus should only be on getting the infrastructure done.

Implementation Brief

• Convert existing QUnit tests (already complete in #437)
• Remove node-qunit-phantomjs and qunit packages (already complete in #437)
• For functions that currently rely on global variables (eg. window.location), using dependency injection/mocking via an extra, private variable for said function's API would allow us to better test the functions without messing about too much with Jest globals.
• Investigate issues with localisations loading in Jest (I think QUnit ran in the browser, correct? The way locales are being loaded is messing with numberFormat tests.) Looking at the code though, the locale is directly supplied so it might be an issue with how the test was migrated (if it's new) or an issue with the expected-vs-actual function signature of the function we're trying to test.
• Remove /tests/qunit/* from .eslintignore.
• Add a jest --watch npm command, as running npm run test:js -- --watch is cumbersome. It might be nice to replace the existing npm test to use jest --watch, actually.
• I would encourage a refactor of tests like https://github.com/google/site-kit-wp/blob/81a2a86bade49e424dc4c29bfb0d12792b39d18f/assets/js/util/test/getReAuthUrl.js#L6—the current approach reduces readability in favour of less code repetition, but making tests readable is vital to keeping them useful. Understanding what's happening in test files like this requires a lot of context-switching between the variables at the top and the way the function transforms them. The mechanics of this test file are more complex than needed and reduce readability. Especially given the low number of tests actually run, I would migrate to clearer tests with hardcoded values and clearer spec names (this will increase grep-a-bility of the test specs as well).

Changelog entry

• Introduce Jest for JS unit tests, migrate existing tests, and improve various related infrastructure components.

[felixarntz]

@swissspidy In the PR you're stating:

So far it works really well. I only had to skip a few tests because of various reasons:

a) Node is missing some locale data
b) Some tests were inconsistent
c) Tests have weird dependencies on global variables

How many / which tests did you remove because of that? The reasoning is valid, but we wanna make sure we don't lose too much coverage, especially for more crucial functions.

[swissspidy]

I did not remove any tests. I implemented them, but they are not passing because the reasons cited above. See here:

https://github.com/google/site-kit-wp/pull/437/files#diff-5c7a734b3d0401947065635e43c39fe8R79-R85
https://github.com/google/site-kit-wp/pull/437/files#diff-fd794c928328b1555e56258c92a91f68R16-R27
https://github.com/google/site-kit-wp/pull/437/files#diff-f850330e71c052ce6f013eb93aaf2fbbR29-R37
https://github.com/google/site-kit-wp/pull/437/files#diff-f4eb66e0fe7203be901ef442eb2e1e52R33-R41
https://github.com/google/site-kit-wp/pull/437/files#diff-56770681d65223e26fa5ace24101d490R15-R38
https://github.com/google/site-kit-wp/pull/437/files#diff-13624d606176271e11732697bd1ca93dR23-R44

[aaemnnosttv]

@swissspidy your links don't work for me, it stays at the top of the page 😕

Actually the JS Tests job passed, it's the E2E tests that all failed for some reason

https://travis-ci.com/google/site-kit-wp/builds/124611432

[swissspidy]

our links don't work for me, it stays at the top of the page

Alternatively just scroll down and Cmd+F for it.skip.

Actually the JS Tests job passed

Well they pass because the failing ones are being skipped 😉

it's the E2E tests that all failed for some reason

Since the branch is not up-to-date with develop anymore I would ignore that for the moment.

[tofumatt]

I've added some thoughts to the IB; I only encouraged 14 failed tests when I ran the branch in #437 locally. Many of those tests are issues with numberFormat's locale handling, and overall this seems a large way toward being done assuming the rebase on top of develop isn't too messy. I tried to rebase and noticed much of the conflicts were around the module imports. That's tedious but not difficult work.

Seems like finishing this transition could be a medium-sized task from what I've seen of it thus far.

[tofumatt]

Forgot to mention: I'm un-assigning myself here; if @swissspidy or @aaemnnosttv have more to add to the IB please feel free; if not I think this can move to IB review.

[swissspidy]

Looking at #437 again, there are some issues with the webpack and Babel configuration, causing some troubles with ESLint and also the mapping of @wordpress/* component imports to wp.* globals (and thus build failures. Might be worth reflecting that in the IB, even though #437 covers a large part there already.

Also, even though #437 covers a lot here already, I feel like this is rather on the large size rather than medium.

---

Looking at the code though, the locale is directly supplied so it might be an issue with how the test was migrated (if it's new) or an issue with the expected-vs-actual function signature of the function we're trying to test.

FWIW I think the issue there is simply that Node does not have these locales installed by default. That's why the tests are failing. We'd need to install the ICU locale data for tests to work

[aaemnnosttv]

I feel like this is rather on the large size rather than medium.

@swissspidy we should perhaps split this into two tasks: one for the ESLint config and import refactoring (https://github.com/google/site-kit-wp/issues/520#issuecomment-535423900), and keeping this one for the Jest foundation and migration from QUnit.

Unless there's a reason why these need to happen as part of the same ticket (although I think the first would be a prerequisite to this one), I think it may be better to split them.

[swissspidy]

520 / https://github.com/google/site-kit-wp/issues/520#issuecomment-535423900 is rather unrelated to this 🤔

I changed the imports in #437 mainly because plenty of functions tested via Jest reside in files that import wp.* components. That global doesn't exist in a Node/jsdom environment.

Sure, you can split up setting the foundation (without any tests) and then the migration into separate PRs but I don't see a big advantage in that.

[aaemnnosttv]

I changed the imports in #437 mainly because plenty of functions tested via Jest reside in files that import wp.* components. That global doesn't exist in a Node/jsdom environment.

That makes sense.

I wasn't suggesting splitting foundation and test migration into separate issues. I was under the assumption that there was other work needed to be done such as the imports (also part of overall JSR guidelines) but if the changes alluded to in #520 are only jsDoc related then it's fine. I thought there was perhaps other changes that would be required by that update.

I'll bump this to a large per your recommendation 👍

[aaemnnosttv]

@tofumatt Moving this one back for now just to make it more clear what we're actively working on.

[aaemnnosttv]

QA ✅

• [x] The plugin should contain necessary configuration to run Jest unit tests.

• [x] Travis-CI should run Jest unit tests present, and they should make the build fail as expected when they fail

  • Travis command: https://github.com/google/site-kit-wp/blob/cb0b1e104b0c6348af2ffdc58d1396aa9b585cda/.travis.yml#L72

  • Changing an existing test to fail in an expected way, and running
    npm run test:js || echo 'exiting!' _fails and outputs exiting! as expected_
    

    
    

     FAIL  assets/js/util/test/getQueryParameter.js
    getQueryParameter
    ✕ should return the correct query param values (16ms)
    
    ● getQueryParameter › should return the correct query param values
    
    expect(received).toStrictEqual(expected) // deep equality
    
    Expected: "blah"
    Received: "bar"
    
      11 |  it( 'should return the correct query param values', () => {
      12 |      let location = createLocation( 'foo=bar&x=1' );
    > 13 |      expect( getQueryParameter( 'foo', location ) ).toStrictEqual( 'blah' );
         |                                                     ^
      14 | 
      15 |      location = createLocation( 'bar=foo&x=1' );
      16 |      expect( getQueryParameter( 'bar', location ) ).toStrictEqual( 'foo' );
    
      at Object.<anonymous> (assets/js/util/test/getQueryParameter.js:13:50)
    

    
    

• [x] Any test directories should be excluded from the plugin production build (e.g. npm run release-zip).
  find dist -type d dist dist/assets dist/assets/svg dist/assets/css dist/assets/images dist/assets/js dist/assets/js/externals dist/assets/vendor

Optional: Migration of QUnit tests is generally not a requirement for this issue because of the work it entails. However, because of much (all?) of it already being done in #437, we can continue working towards the goal of eliminating QUnit.
QUnit tests should continue to be run in Travis-CI unless actually all QUnit tests have been migrated to using Jest. If so, the entire QUnit infrastructure and references should be removed from the codebase.

• [x] the entire QUnit infrastructure and references should be removed from the codebase
  
  
  
  • The only reference to qunit left anywhere is here (insignificant)
    
    https://github.com/google/site-kit-wp/blob/cb0b1e104b0c6348af2ffdc58d1396aa9b585cda/.eslintignore#L6
  
  

---

Moving to Approval