feat(tests): Setup unit testing framework

[hareetd]

Related #192

This PR sets up a unit testing environment and adds two tests. The CLI command to run the test suite is npm run test.

Background

Jest is a Javascript testing framework that allows us to create, run and structure our tests, similar to JUnit on the backend. However, unlike JUnit, Jest also provides built-in mocking capabilities.

Since our app is React-based, we’ll need additional libraries that make it easier to test our React components. This is where React Testing Library (RTL) comes in. It gives us the ability to render our components into their HTML Document Object Model (DOM) representation (i.e. what the user “sees” when they visit a webpage) and query/assert on the nodes and objects in the DOM. For example, a node could be a <button /> element that we query for and perform assertions or actions (such as a “click”) on (i.e. what the user “does” when they interact with the -web UI).

I had originally intended to use another React unit testing library called Enzyme. You’ll notice that it’s already included in the dependencies for -web, unlike RTL. I think the frontend was created using a template React application as a base, such as Create React App, which is why a lot of dependencies needed for testing are already included in package.json, such as Jest and Enzyme.

Enzyme also involves rendering React components but gives you the additional ability to test the implementation details of components through working directly with component instances. This provides access to the state, props, and children components of the component under test. This is in stark contrast to RTL where the guiding principle is that implementation details should be avoided in favour of more maintainable tests that are conducted from the perspective of the user (i.e. the user has no access to the implementation details of a component, they simply see and interact with the rendered HTML).

What this means is that Enzyme’s functionality is closer to what we typically consider unit testing with respect to the backend, where the object under test can be isolated and its internal implementation details tested. RTL, however, is almost closer to integration testing because its tests don’t care about implementation details but rather how components function as a whole from the user perspective. For a more detailed discussion see React Testing Library vs. Enzyme.

Although the flexibility of using both Enzyme and RTL would have been nice, unfortunately, it looks like there is no Enzyme support planned for React 17 so we will have to stick with RTL.

Mocking

As mentioned earlier, Jest provides built-in mocking. It’s fairly powerful and from my experience using it so far, it should be sufficient for our testing purposes. However, if we find this isn’t the case as more tests are added, a standalone mocking library such as Sinon is also an option.

As for deciding what should be mocked, at first what I tried was to mock out all child components inside the component under test. The idea was to mirror how we unit test on the backend where all the dependencies of the Java class under test are mocked out in order to isolate the class as a unit. However, I realized that this goes against RTL’s guiding principle that “the more your tests resemble the way your software is used, the more confidence they can give you”. The creator of RTL expands on this in an article on mocking, where he states that in the majority of his UI unit tests, except for network calls and animations, everything else is left unmocked and so uses the real production code.

I believe a happy balance for our unit tests is to mock out API calls, child components that are of our own making (since they’ll have their own unit tests), and the shared services that we propagate using the ServiceContext across many components. If possible, any third-party child components, such as those belonging to Patternfly, should be left unmocked so that the rendered HTML DOM our tests work with more closely resembles how our components are actually rendered on the user’s webpage. The downside of this approach is that tests will be dependent on the underlying implementations of these third-party components. I’d appreciate some feedback on this as you review the tests I’ve added.

Snapshots

Snapshot testing helps ensure that we stay on top of any changes to our UI. It’s a complement to regular unit testing, in which we render React components, take a serialized snapshot of the result, and compare it to a reference snapshot file to see if anything has changed. Snapshot files are committed to version control alongside their corresponding tests and are included in the review process.

Instead of RTL, we use the create function from the react-test-renderer library to render our components for snapshot testing. This is because create works with the React virtual DOM, a lightweight abstraction of the actual HTML DOM representation of the component, consisting of pure Javascript. The virtual DOM can be queried but not interacted with, which is fine since we’ll only be taking snapshots of it. For a more detailed discussion on the virtual DOM, see here.

When the Jest test suite runs, a new snapshot will be created for every component under test and compared to the reference snapshot in version control (the first reference snapshot is created the first time a snapshot test is run for a specific component). If there is any discrepancy between the two snapshots a diff will be output to the command line. From here, it's up to you to determine whether the difference is due to a bug or an intentional implementation change. This may warrant updating or adding more unit tests. When you’re satisfied with the reasons behind the changed snapshot, you can update it to be the new reference snapshot by running the following command:

npm run test -- -u -t="SPEC_NAME"

Where the -u flag tells Jest to update the snapshot and the -t flag specifies which test to update it for. SPEC_NAME is matched against the string passed in to the describe call of the test file in question. For example, in Recordings.test.tsx the unit tests are housed inside of the describe(‘<Recordings />’, ….) block. In order to update the snapshot for Recordings.tsx, you would pass -t=”<Recordings />” to the above command. Remember to commit and push the new reference snapshot to version control.

Changes

.github/workflows/ci.yaml

Add unit testing to the CI workflow.

TESTING.md

A condensed version of this PR body for future, easy-to-access information on testing.

jest.config.js

This file allows us to configure Jest. It seems most of the settings have been there since the initial -web project was created but I’ve made two changes.

By removing collectCoverage: true, code coverage is no longer collected automatically. Instead, the decision to do so is delegated to the Jest CLI configurations in package.json.

ts-jest is a library providing Typescript support for Jest. By setting isolatedModules: true, Typescript files are compiled separately from each other as isolated modules. This helps resolve an issue where tests take unnecessarily long to complete (minutes timescale instead of seconds for two, small tests). The solution was taken from here. It’s important to note that this setting speeds up performance at the expense of type-checking and other features, which may lead to other issues. It seems there are alternative solutions but I would need to investigate them further since I’m not familiar with the Typescript compilation process.

transformIgnorePatterns: ["/node_modules/(?!@patternfly)"] ensures that the PatternFly module in the node_modules folder is transformed (i.e. transpiled) into valid JavaScript. This resolves an issue where Jest fails to parse the import of SearchIcon from @patternfly/react-icons/dist/esm/icons/search-icon.js, used in src/app/Recordings/RecordingsTable.tsx. You can trigger the parsing failure yourself by removing the transformIgnorePatterns setting and commenting out the mocks for either the ActiveRecordingsTable or ArchivedRecordingsTable inside Recordings.test.tsx (lines 96-112), then running the test suite. This will show a detailed error message with other potential solutions.

license-config.json

Exclude adding the Cryostat license to TESTING.md and snapshots (the licensing interferes with accurate snapshot diffing).

package.json

I've added two different Jest CLI configurations; test is for local environments and test:ci is for the Github CI workflow.

When the command npm run test is entered in the CLI, jest --maxWorkers=50% --coverage=true is what’s actually run. The --maxWorkers flag determines how many threads the worker-pool will spawn for running tests. After this discussion with Andrew, I've settled on 50% (i.e. half of the available CPU threads) for both local and CI testing; we can always adjust this if we find tests are running slow. --coverage=true tells Jest to produce a code coverage report along with the test results.

The first time you run the test suite will be the slowest. However, subsequent runs will be faster since Jest uses a separate cache in each thread to speed up the process.

For the test:ci configuration, I've decided to not collect code coverage data since it's a fairly resource-intensive task and doesn't seem necessary to include if the local testing will do it instead.

The remaining changes are the dependencies I’ve added in order for us to use RTL and snapshots in our tests.

src/app/Recordings/Recordings.tsx

If custom id props are not supplied to the PatternFly Tabs and Tab components, random, dynamic ids are generated instead and used throughout these components. This leads to an issue where snapshots of the Recordings component will change upon every render, triggering a snapshot diff when in reality the only thing that hmesses as changed is the randomly generated string values. This solution is recommended by the PatternFly maintainers.

src/app/snapshots/app.test.tsx.snap

I deleted this snapshot and its corresponding test (see my reasoning below).

src/app/app.test.tsx → src/test/About/About.test.tsx

I think I messed up the Git versioning here since I didn’t intend this change to be a file replacement but rather a separate deletion and addition. Regardless, the reason I’m deleting app.test.tsx and its corresponding snapshot above is because the component under test, specifically App in src/app/index.tsx, consists solely of nested components with no major props passed in (besides the defaultServices) or any other React code. My understanding is that this component is the foundation of our frontend application, handling the provision of the various shared services, as well as the routing and overall layout through its nested components. It would require an excessive amount of mocking in order to get the unit test for this component to work, but more importantly, since we will eventually have unit tests for the AppLayout and AppRoutes components, there isn't really anything to test for if we do isolate this component as a unit. For this reason, I believe testing this component would be better suited to end-to-end testing. I’d appreciate others’ thoughts on this decision, especially yours, Andrew, since it looks like you added this test during the rewrite of the frontend client from Angular to React. I’m also open to revisiting this in the future once I have more experience with testing. You’ll notice the test is written with Enzyme so it would have to be changed to use RTL.

About.test.tsx is the test I wrote for the About component in src/app/About/About.tsx. It’s fairly simple since all the About component does is pass in some props to its nested child components. The BreadcrumbPage and AboutDescription are mocked out and replaced with a simple div, with props for the BreadcrumbPage preserved. I’ve left the various PatternFly components unmocked.

The tests are grouped together using the describe function, with individual tests denoted using the it function call. The first test creates a snapshot of the About component using the react-test-renderer and uses Jest’s assertion and matching functionality to ensure it matches the reference snapshot.

The second test renders the About component into its HTML DOM representation using RTL and makes assertions to determine whether the correct props have been passed into its child components. The screen object allows you to query the rendered document. It’ll be easier to see why my queries are structured the way they are if you insert screen.debug() after the render(<About />) call, then re-run the test suite. This will output the HTML DOM document to the command line so you can see exactly what we’re querying against.

It’s also possible to test the correctness of props passed into mocks by directly interacting with the mock instance. Using the BreadcrumbPage as an example:

const breadcrumbPageMock = BreadcrumbPage as unknown as jest.Mock;
expect(breadcrumbPageMock).toHaveBeenCalledWith(expect.objectContaining({ pageTitle: 'About' }), expect.anything());

Where the first line obtains the mock instance and the second line asserts that the correct pageTitle prop was passed in. However, keeping in mind RTL’s guiding principles, from the user's perspective they don’t know anything about the underlying props used. The only thing they see and interact with is the HTML DOM representation, which is why we should stick with using only RTL queries when possible. However, if you find that a prop you would like to test for is swallowed up by the render and not represented in some way in the HTML DOM, feel free to use this approach to test for the prop.

An important thing to note is that the src prop for the Cryostat logo <img /> is the string “test-file-stub” instead of the @app/assets/logo-cryostat-3-horizontal.svg file used in the About component. This is because line 30 in jest.config.js ensures that during tests any file with one of the listed extensions will be stubbed out with the string “test-file-stub”, which is itself defined in /__mocks__/fileMock.js.

With regards to mocking, line 58 (...jest.requireActual('@app/About/AboutDescription'),) may be a bit confusing. This ensures every other exported object in AboutDescription.tsx besides the AboutDescription component (which gets mocked in the following lines) remains unmocked, allowing us to import and use the actual CRYOSTAT_TRADEMARK string in the query on line 81.

src/test/About/snapshots/About.test.tsx.snap

The reference snapshot for the About component. You’ll notice the BreadcrumbPage and AboutDescription are represented using their mocked versions while the PatternFly components consist of their actual implementations.

src/test/Recordings/Recordings.test.tsx

There are two things to note about the ApiService mock on line 78. The first is that the mock implementation uses of from the rxjs library to mock out the return value for the ApiService.isArchiveEnabled() function. Because of this, it’s important that the import { Recordings } from '@app/Recordings/Recordings' be done after the import { of } from 'rxjs', otherwise Jest will complain that of is undefined. The way Jest works is that any jest.mock(...) calls are automatically hoisted to the top of files, above the imports. This ensures that when modules are imported, Jest knows to replace the real implementations with the mocked versions. However, the actual implementations of mocks aren’t processed until the component under test is imported, which is why it’s important to do this import last so that any imported modules used inside the implementations are defined already.

Second, the isArchiveEnabled function’s return value is mocked five times. Each of these mock definitions corresponds to a separate render call, one for each of the five unit tests defined below, in the order that they are written. The final definition uses mockReturnValue instead of mockReturnValueOnce, meaning from the fifth render call onwards, isArchiveEnabled will return of(true). Mocking the return values on a per-test basis (i.e. inside the it block) using jest.doMock is also possible, but requires a bit more work.

You’ll notice that the snapshot testing is done differently than in About.test.tsx. If you take a look at line 51 of src/app/Recordings/Recordings.tsx, the isArchiveEnabled API call is wrapped inside React.useEffect, which gets run only after the render of a component is committed (or “painted”) to the screen. My understanding is that since the create function from the react-test-renderer produces the React virtual DOM representation of a component, nothing is painted to the screen (unlike in RTL, which works with the real HTML DOM). This means the useEffect is never triggered and with the default archiveEnabled state set to false, the snapshot of the Recordings component will contain considerably less information than when archiving is enabled. Fortunately, the react-test-renderer provides the asynchronous act function, which ensures that any state updates and enqueued effects will be executed, allowing us to have a reference snapshot of the Recordings component with archiving enabled.

In the it('handles updating the activeTab state',...) test we use userEvent from a companion library for RTL to simulate the user clicking the archived recordings tab, in order to check that the Recordings component is properly updating the activeTab state. Note that we’re not actually interacting directly with the state but instead interacting with the HTML DOM representation to induce state changes, all from a user perspective. Remember to use screen.debug() for a better understanding of the queries used.

src/test/Recordings/snapshots/Recordings.test.tsx.snap

The reference snapshot for the Recordings component.

test-setup.js

If you look at line 38 of jest.config.js, you’ll notice the setupFilesAfterEnv flag is set to run test-setup.js. Essentially, this allows you to use test-setup.js to configure or set up the testing framework before any tests are run.

I’ve decided to mock out the shared services here since they’re used so frequently throughout the various components in our application. In this case, both the Recordings and About components use the ServiceContext. The Recordings component does this directly while the About component uses the context through the unmocked parts of its AboutDescription child component. We can use jest.requireActual when we want to work with the real implementations of the shared services, such as during their own unit tests.

yarn.lock

After updating package.json with the needed dependencies, I ran yarn install in order to update yarn.lock which resulted in the changes seen here. However, I’m unsure if this was the correct approach since quite a few dependencies have been updated as a result so I’d appreciate any feedback on this.

Edit: Andrew provided the correct approach to updating the yarn.lock file here. I followed the instructions and the updated yarn.lock is correct now.

[andrewazores]

After updating package.json with the needed dependencies, I ran yarn install in order to update yarn.lock which resulted in the changes seen here. However, I’m unsure if this was the correct approach since quite a few dependencies have been updated as a result so I’d appreciate any feedback on this.

I think the way to go about fixing this might be (I'm not 100% confident, so let me know how it works out):

1. rm -rf node_modules
2. git checkout package.json --> make notes of the new deps you needed before doing this
3. git checkout yarn.lock
4. yarn yarn:frzinstall --> re-install all usual dependencies according to lockfile versioning
5. yarn add --dev --exact foo@1.2.3 --> replace foo with each of the new deps you need

After that, other users and the CI build can do the usual yarn yarn:frzinstall to get dependencies matching the new lockfile, without other libraries also being upgraded by this PR.

[andrewazores]

For this reason, I believe testing this component would be better suited to end-to-end testing. I’d appreciate others’ thoughts on this decision, especially yours, Andrew, since it looks like you added this test during the #81.

For sure, this makes perfect sense to me. The existing tests before this PR were basically useless and of no real value anyway - they did essentially all just come from the Patternfly React seed template application, and were never updated since.

I’m also open to revisiting this in the future once I have more experience with testing. You’ll notice the test is written with Enzyme so it would have to be changed to use RTL.

I do think it would be best to just use RTL everywhere, especially if Enzyme won't be supported later on anyway, but that can wait for a follow-up PR.

[andrewazores]

This looks fantastic overall, thanks for doing all that research and write-up. Could you boil down that PR body slightly and create a new file like TESTING in the root of the repo that has some of those tips and hints in it? Details like how the import order matters, how to update snapshots, etc.

[hareetd]

I think the way to go about fixing this might be (I'm not 100% confident, so let me know how it works out):

1. `rm -rf node_modules`

2. `git checkout package.json` --> make notes of the new deps you needed before doing this

3. `git checkout yarn.lock`

4. `yarn yarn:frzinstall` --> re-install all usual dependencies according to lockfile versioning

5. `yarn add --dev --exact foo@1.2.3` --> replace foo with each of the new deps you need

After that, other users and the CI build can do the usual yarn yarn:frzinstall to get dependencies matching the new lockfile, without other libraries also being upgraded by this PR.

Thanks, I'll try this out. Right now the test suite is failing during the build due to a webpack error which I'm fairly sure is due to the change in various dependencies. However, the build runs fine if I change the webpack version in package.json from ^5.38.1 to exactly 5.38.1. Hopefully your tips can resolve this.

This looks fantastic overall, thanks for doing all that research and write-up. Could you boil down that PR body slightly and create a new file like TESTING in the root of the repo that has some of those tips and hints in it? Details like how the import order matters, how to update snapshots, etc.

Thanks, it was a good learning experience! A TESTING file is a good idea; I'll limit the PR body to the more general background and file specific things I've done. Let me know if I should condense it further once I have the TESTING file added.

[andrewazores]

Thanks, it was a good learning experience! A TESTING file is a good idea; I'll limit the PR body to the more general background and file specific things I've done. Let me know if I should condense it further once I have the TESTING file added.

You can leave the PR body as it is, no need to remove content from there. I just want to make sure all those valuable tips and tricks get preserved somewhere easily visible for future reference after this is merged.

[andrewazores]

Hey, it builds again. Nice!

[andrewazores]

Could you update .github/workflows/ci.yaml and add a step to run the new tests, too?

[hareetd]

Yeah, the steps you gave worked. The yarn.lock diff is now much more reasonable in size as well.

[jan-law]

Great work! Your notes were really helpful while I was reviewing this PR.

[andrewazores]

LGTM. Anyone else?

[hareetd]

@Josh-Matsuoka mentioned in this week's team meeting that he's still planning on taking a look at this PR since he needs to add tests for his Agent Plugin UI so we could wait to merge until he has a chance to review?

[Josh-Matsuoka]

LGTM