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

Set up testing environment #682

Open
wants to merge 23 commits into
base: nfr/ts-major-refactor
Choose a base branch
from

Conversation

ManpreetSL
Copy link
Contributor

@ManpreetSL ManpreetSL commented May 4, 2023

Currently, the project doesn't have working test functionality. We want to be able to use Jest and React Testing Library to add unit and integration tests to the app. However, there are a few build errors such as a compatibility issue between MUI and React 18.

This PR fixes build errors, and then sets up and configures Jest and React Testing Library, along with adding some basic tests to ensure that the environment works properly.

@ManpreetSL ManpreetSL changed the title Add structure for testing Set up testing environment Jul 14, 2023
@ManpreetSL ManpreetSL marked this pull request as ready for review July 14, 2023 04:21
Copy link
Member

@Harjot1Singh Harjot1Singh left a comment

Choose a reason for hiding this comment

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

Very good progress!

.eslintrc Outdated Show resolved Hide resolved
Comment on lines 5 to 7
global.fetch = jest.fn().mockResolvedValue( {
json: () => Promise.resolve( {} ),
} )
Copy link
Member

Choose a reason for hiding this comment

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

Be careful with mocks like these! I think it's ok for now to get the tests to pass, but we should see if there's a more established/clear method for mocking fetch etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have a better way, let me know... Google wasn't very helpful with this.

apps/frontend/src/Settings/Hotkeys/index.tsx Outdated Show resolved Hide resolved
}

describe( 'hooks', () => {
describe( 'useCopyToClipboard()', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This suite ends up testing a lot of the internals of the function, rather than outcome! You can try to test the outcome of hooks by setting up a scaffolding React component, and then asserting against the outcomes, as we'd normally do :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So with this, I'm looking to implement:

  • Check what was copied to the clipboard
  • Check for a snackbar and its text to appear on the screen using RTL
    Does that sound sensible?

Comment on lines +3 to +9
const isMacMockGetter = jest.fn()

jest.mock( './consts', () => ( {
get isMac() {
return isMacMockGetter()
},
} ) )
Copy link
Member

Choose a reason for hiding this comment

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

Very interesting approach! I think there might be a simpler option:

import * as consts from './consts'

consts.isMac = true

might work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cannot assign to 'isMac' because it is a read-only property.

Comment on lines +15 to +18
coverageDirectory: 'coverage',

// Indicates which provider should be used to instrument code for coverage
coverageProvider: 'v8',
Copy link
Member

Choose a reason for hiding this comment

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

Are these options required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were created from the initial Jest setup, I think. Do you want me to remove them?

"target": "ESNext",
"lib": ["ESNext"],
"useDefineForClassFields": true,
"target": "es5",
Copy link
Member

Choose a reason for hiding this comment

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

Do you know which fields are required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I figure that out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants