Skip to content

test: add missing test for suggestions #1215

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 8, 2023
Merged

test: add missing test for suggestions #1215

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
652bad8
test: add missing test for suggestions
MatanBobi Feb 17, 2023
6d62cc6
fix: cleanup in afterEach
MatanBobi Feb 17, 2023
ab0f71a
Check we actually suggest
eps1lon Mar 8, 2023
f48356c
TODO: reword+fixup
eps1lon Mar 8, 2023
183d075
Merge branch 'main' into tests/add-missing-test-for-suggestions
eps1lon Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/__tests__/suggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ test('should not suggest when suggest is turned off for a query', () => {
).not.toThrowError()
})

test('should suggest when suggest is turned on for a specific query but disabled in config', () => {
configure({throwSuggestions: false})
renderIntoDocument(`
<button data-testid="foo">submit</button>
<button data-testid="foot">another</button>`)

expect(() => screen.getByTestId('foo', {suggest: true})).toThrowError()
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really testing whether we're suggesting anything. Just that an error is thrown. I'll see if a snapshot makes more sense

configure({throwSuggestions: true})
Copy link
Member

Choose a reason for hiding this comment

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

These setup+cleanup tasks need to be in a beforeEach/afterEach. Otherwise the whole test run would be tainted if this single test fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is the only test needing them, I didn't want to add the overhead of running that code in a 'beforeEach' and 'afterEach' so I changed it at the beginning and returned it to what the 'beforeAll' hook defined. Do you think it will be better in this case to do it in the hooks instead?

Copy link
Member

Choose a reason for hiding this comment

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

every setup/cleanup for tests needs to be in hooks for the the reason I explained: once a test fails, the whole test run is tainted because because its cleanup didn't run. This can be super confusing if you try to debug the last test that fails when the only reason for that test failure is the first test.
Concretely:
say we have test 1 and 2. 1 needs environment A and 2 needs B. 1 sets up env A but fails. Now 2 also fails because it has env A+B. If you don't check the implementation of test 1 first, you might start debugging test 2 even though there's nothing in test 2 that needs fixing.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, that's a good point, thanks.
I've put the cleanup as part of the afterEach that was already there but I believe that the setup should remain within the test as it's the only test that needs this setup. Wdyt?

})

test('should suggest getByRole when used with getBy', () => {
renderIntoDocument(`<button data-testid="foo">submit</button>`)

Expand Down