LCV container tests

[srallen]

Package: lib-classifier

For #650

Describe your changes:
A first step toward the refactor by adding tests to the LCV container component. I plan on moving the store connection from the LCV to the LCV container and I want to make sure what is existing doesn't get broken in that process.

I have a single failing test that is a bit of a mystery to me at the moment. It calls the onReady prop on successful load passes if isolated with only, but fails if run altogether with the block. I figured out that I was missing a config for nock to work for multiple tests.

Review Checklist

General

• Are the tests passing locally and on Travis?
• Is the documentation up to date?
• Is the changelog updated?

Apps

• Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
• Does it work on mobile?
• Can you rm -rf node_modules/ && npm install and app works as expected?

[eatyourgreens]

I have some questions about the changes to the LCV container component. I'm not sure about modifying the React lifecycle methods in order to get the tests to work.

If methods are modified to return a value then there should be a default return value, outside of any if blocks, for consistency.

[srallen]

Could we have waiting until merging #810 until I had a chance to respond?

[srallen]

We discussed potentially using the onReady prop to spy on instead.

[srallen]

A few minutes of experimentation I've found that spying on onReady isn't useful for testing error states since onReady won't be called. We could spy on onError, but we'll have the same problem of figuring out how to tell the test to wait for the async calls to resolve.

I experimented with disabling the lifecycle methods and found that it worked for testing on waiting for a single promise to resolve, but not a chain of them. For example, my test for it 'should error if the location request response fails' is failing doing this because the test expectations run before the await for the http request resolves.

I really think returning the promise from componentDidMount is cleanest and most reliable in this case. I've found another example doing a similar approach: https://medium.com/@manastunga/unit-testing-api-calls-in-react-enzyme-and-jest-133b87aaacb4

One last approach could be flushing the promises, which I haven't tried yet: https://medium.com/@lucksp_22012/jest-enzyme-react-testing-with-async-componentdidmount-7c4c99e77d2d

[eatyourgreens]

async componentDidMount seems much less brittle than conditionally returning a Promise if a specific prop is present. You don't have to check whether componentDidMount().then exists before you use it.

Should onError happen in the store, rather than the component? #815 has made me wonder if there should be a common error handler for these components.

[eatyourgreens]

In #815 I'm splitting the viewer tests into without a subject, with a valid subject and with an invalid subject, which might be a useful way to organise the tests here too.

[srallen]

Should onError happen in the store, rather than the component? #815 has made me wonder if there should be a common error handler for these components.

If ready is renamed to be loading and used the async states and then we also stored the error message in the store, then that will be more consistent with the other store APIs. I'd be for this refactor.

[srallen]

async componentDidMount seems much less brittle than conditionally returning a Promise if a specific prop is present. You don't have to check whether componentDidMount().then exists before you use it.

I'll give this approach a try. I can see your point about using a return for the other promise being inconsistent since it is within a conditional while making componentDidMount async will guarantee that a promise is returned.

[eatyourgreens]

I'm going to update #815 to add loadingState to the subject viewer store.

[srallen]

This will be updated to use the improved error handling introduced in #815

[srallen]

This is ready for review again. It's been updated to use the error handling in the subject viewer store.

[eatyourgreens]

I have some questions about changes to the error handling in the component itself. There's a couple of places where I think the code is catching errors then failing silently, where previously errors were logged to the console. I'm not sure if that's intended.

In the tests, you could stub the onReady and onError callbacks to check expectations and call done() after they've run, which might make the code cleaner. That would guard against any cases where chained promises mean your initial promise resolves before the test has finished running.

[srallen]

@eatyourgreens I've tidied this up a bit. I think it's ready for review again.

[eatyourgreens]

Looks good to me. I think we've been talking at cross purposes about how try/catch works. I'm suggesting that requestData should always return a value for rawData. We've enforced consistent returns for functions in PFE, and I've got into the habit of looking for that in code.

If requestData sometimes returns a data object, and sometimes an error or undefined, then that would be inconsistent and require more code to decide how to handle the return value, as you noted. My preference is to just keep the return type consistent for functions.

[eatyourgreens]

There was a stray .only() in the tests, so I've removed it using the PR suggestion/commit feature.

[srallen]

@eatyourgreens this is ready for review again