Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(tests): Add credentials table test #408

Merged
merged 13 commits into from
Apr 26, 2022

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Apr 13, 2022

Related #403

Adds unit tests for the Stored JMX Credentials Table and some refactoring to allow Jest access to the table components via imports and component labels.

You'll notice that the enums in StoreJmxCredentials were changed to "'TargetCredentialsStored' as NotificationCategory.TargetCredentialsStored". This is a workaround due to the fact that Jest doesn't support exported const enums when testing (link to issue)
Nevermind, fixed the messageKeys import by adding ...jest.requireActual('@app/Shared/Services/NotificationChannel.service'), to the mocked NotificationChannel service.

To make the tests easier to read, I also split up the 'Delete credentials' test case into two tests, ie 'removes the correct table entry when a deletion notification is received' and 'makes a delete request when deleting one credential'. The first test receives a mocked notification before asserting that the table state was updated, and the second test clicks the Add/Delete button before asserting that the API calls were made. (The equivalent API call assertion for adding stored credentials isn't tested here because the call is made in AuthModal.)

@jan-law jan-law added the chore Refactor, rename, cleanup, etc. label Apr 13, 2022
@jan-law jan-law requested review from andrewazores and hareetd April 20, 2022 21:26
hareetd
hareetd previously approved these changes Apr 20, 2022
Copy link
Contributor

@hareetd hareetd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Judging from your mock on line 94, it seems the NotificationChannel.messages subscription is invoked three times in StoreJmxCredentials. For my own understanding, could you explain why this happens? I thought there would be only two invocations (one for a stored notification and the other for a deleted notification) once the render is complete and the useEffect calls are triggered.

@hareetd hareetd marked this pull request as ready for review April 20, 2022 23:10
@andrewazores
Copy link
Member

Check on that latest commit please - the package-lock.json exploded.

@jan-law jan-law force-pushed the jmx-cred-unit-test branch from 8d7037c to f6a8d35 Compare April 25, 2022 21:03
@jan-law
Copy link
Contributor Author

jan-law commented Apr 25, 2022

I rebased my last commit and removed the extra dependencies - it turns out that you don't need dynamic importing for jest.doMock to work unless you are specifically using import("../components/Component") inside your tests.

@jan-law jan-law force-pushed the jmx-cred-unit-test branch from fcbfa0d to 34a3303 Compare April 25, 2022 21:45
@jan-law jan-law force-pushed the jmx-cred-unit-test branch from 34a3303 to e95d9bb Compare April 25, 2022 21:48
TESTING.md Outdated Show resolved Hide resolved
@andrewazores
Copy link
Member

@jan-law ready to merge?

@jan-law
Copy link
Contributor Author

jan-law commented Apr 26, 2022

yes, ready to merge

@andrewazores andrewazores merged commit 83349b4 into cryostatio:main Apr 26, 2022
@jan-law jan-law deleted the jmx-cred-unit-test branch April 26, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants