Refactor tests with nock and enzyme

[pierluigi]

The Uber "Test Refactoring" PR from Hell

feat. Nock & Enzyme
This (partially) fixes mesosphere/marathon#2099

Note

This PR mostly solves the tech debt that prevented us from moving forward with a better testing strategy. I believe that incorporating these tools will allow us to achieve the following [quoting from @philipnrmn]

• audit the existing tests, organise them by scenario
• separate unit tests from behavioural tests such that unit tests are clearly associated with the files they test
• ensure that behavioural tests run with a DOM context to avoid the shallow renderer and children[0].props.children[0].props.children[0] type chains
• add coverage numbers
• write and document some example tests
• create jobs for areas where tests are missing
• get a working DCOS integration test runner set up
• write some integration tests

Introduction

This PR refactors all of our tests to achieve the following top-level objectives:

1. introduce nock which simulates API endpoints with no side-effects
2. get rid of the HttpServer mocking utility (responsible for flakey tests) and convert all API-enabled tests to use nock
3. introduce enzyme which acts as a wrapper for various rendering modes
4. refactor all our tests to make use of the shallow rendering method
5. rewrite/add a few DOM-enabled tests to demonstrate the mount rendering method and its capabilities

API testing with Nock

A typical Nock response looks like this:

nock(config.apiURL)
  .get("/v2/apps")
  .reply(200, {message: "Whazzaa!"})

It supports all HTTP verbs (.post(), .put(), .delete() etc.) and the 2nd reply() argument can be an object or a function, which allows us to perform request body parsing etc as needed. One thing to know is that nock can be very nitpicky. More on this later.

Nock also supports a lot of very useful features, which we might consider using in the future. One of these is NockBack which allows us to record API sessions and replay them. This might come in handy in certain scenarios, especially with streaming endpoints.
Another great one is .delay() https://github.com/pgte/nock#delay-the-connection which can be helpful to simulate network problems.

Nock is... nitpicky

There are a few things one needs to keep in mind when writing tests using nock. I will try to summarise:

1. For each nock() route, there MUST be one and only one request. Memorise this like a mantra. Seriously.
2. ...however it's possible to .persist() and .cleanAll(), or even to do things like .twice() and .thrice() if needed. RTFM.
3. Routes with query params need special treatment (.query()): 0c8b34f#diff-579344c36e13e3413d0cabe07ec62de1R52
4. It's always possible to be lazy and just .filteringPath((path) => "/").get("/") to just setup a one-time catch-all interceptor if needed. See https://github.com/mesosphere/marathon-ui/compare/fix/2099-tests?expand=1#diff-91d99d1c80dcd9d19091de99597cada7R23 as an example (we should probably kick this out and intercept the actual path there ;) )

As a result, our tests are more terse and a lot more expressive.

Example: 0c8b34f#diff-579344c36e13e3413d0cabe07ec62de1L35

Instead of intercepting our API calls with a "catch-all" server, this forces us to be explicit about what the request is going to look like. I think that's a big win.

Component testing with Enzyme

Enzyme is great, however it's still under development. It mostly gets the job done nicely, although sometimes I've hit a few brick walls. I am quite impressed with how it makes cheerio transparent and the way it nails DOM-enabling on a per-describe level. Hopefully it's the right bet for our testing future!

The DOM is here! well...now use it!

Refactoring our test was a lot of work, so to avoid going crazy I decided to prevent feature creep and tried to stick to what we had and simply "translate" it to using the most similar enzyme rendering method, shallow(). Outside of this PR, though, it will be a really good exercise to pick some of our tests and re-think them in light of the new possibilities offered by enzyme. For example, we can now finally simulate events on components (ex: simulate("mouseOver")), use setProps to trigger an automatic re-render and traverse the component again, use .find(Component) to find a specific component, etc.
As a POC, I have added a few of these to our existing tests. Here are some highlights:

• .find(Component): 0c8b34f#diff-579344c36e13e3413d0cabe07ec62de1R470
• .setProps(): 0c8b34f#diff-579344c36e13e3413d0cabe07ec62de1R514
• .simulate("mouseOver"): 1a6f21a#diff-6cedd557a588da2c6bd751aaf1fdfabfR1237

Performance

The great news is that enzyme lets you constraint the DOM-enabled tests on a per-describe block. This means that unless we use mount() our tests performance is untouched.
Comparing this branch against current master, I get 459 passing (1s) VS 461 passing (1s) - so performance is pretty much identical.
PS yes, I have unified a few tests as it made more sense, hence the 459 vs 461.

Caveats

About the aforementioned headscratchers I've had with Enzyme, one shortcoming is that components wrapping a ReactRouter.Link fail to render.
This is mostly due to the fact that we're stuck on ReactRouter < 1.0. Apparently, in 1.x the <Link /> element does not depend on the getChildContext anymore, which would allow the component to render normally. ReactRouter docs suggest using a stub (https://github.com/rackt/react-router/blob/57543eb41ce45b994a29792d77c86cc10b51eac9/docs/guides/testing.md) but this unfortunately does not play well with Enzyme. So our only option is to upgrade to 1.0, which is blocked by React 0.14 – see mesosphere/marathon#2710 (comment)

Enzyme also currently does not expose a .unmount() call. I've opened an issue: enzymejs/enzyme#83 but for the moment it's enough to just call the component.instance().willComponentUnmount() lifecycle method where necessary to make sure we unbind our component listeners.

Notes

While refactoring our tests, I came across a few interesting things and wrote down some thoughts. I'll just share them here.

1. the apps.test.js is HUGE. I mean look at it. It really makes refactoring tests a drag because you need to jump back and forth and use .only and xdescribe etc. We should consider splitting our larger tests, as smaller tests are more inviting to the rogue gardener.
2. as much as writing our DOM traversal selectors reads a whole lot nicer with cheerio (which the enzyme renderer wrapper exposes) I'm still not convinced we are doing it right. The coupling is less ugly, but it's still present. I wonder if there is a solution at all...
3. a number of tests had beforeEach blocks that were effectively duplicating requests unnecessarily (see queue.tests.js ex.: 6721d44#diff-f18dec31b5c79e6f92f6012432c0a106L16).

PS

Please feel free to push directly to this branch for stylistic/indentation/cosmetic changes. Also, writing a few tests to get the hang of the various differences between render, shallow and mount really helps understanding the changes introduced in the PR.

[pierluigi]

Assigned to @philipnrmn but of course @orlandohohmeier and @aldipower are MORE than welcome to discuss this!

[philipnrmn]

This is great. Replacing the old testing HttpServer is a huge win; having a DOM is a relief too. Let's schedule breaking the tests up into smaller units soon, I don't think we should do it as part of this PR though.

[aldipower]

Nice

[aldipower]

Nitpick:
The branch could be named refactor/2099-tests instead of fix/2099-tests

[aldipower]

npm install only worked at the second try. At the first try I had conflicts. Strange.

[aldipower]

The jsdom is very slow. Just want to say this. :)

  (uses jsdom)
    Groups
      ✓ are extrapolated from app IDs (128ms)
      ✓ correctly renders in group context (68ms)

[aldipower]

Too bad that there isn't a more convenient way provided by Enzime.

[aldipower]

So nice! Go for it!

[orlandohohmeier]

This is great. Working through all those million line long tests must have been a tough job. Thanks a bunch!

[philipnrmn]

Thanks for that @aldipower!

I'm in favour of 🚢ing it, I'll hit the button tomorrow morning unless I hear objections before then.

[orlandohohmeier]

[aldipower]

🐑 🇮🇹

[philipnrmn]

@Poltergeist, mind giving this a quick once-over as well?

[aldipower]

I'll rebase it with the fresh master.