Tests: incorrect use of .findBy...()

[shaunanoordin]

Incorrect Tests

Noted as of 4062f39

A number of our tests may be returning incorrect results, due to the use of .findBy... selectors.

Example: FinishedAnnouncementConnector.spec.js

it('should show a results link if results page exists', function () {
  render(<DefaultStory />)
  const link = screen.findByLabelText('Announcements.FinishedAnnouncement.seeResults')
  expect(link).exists()
})

This test passes. ✅

  const link = screen.findByLabelText('CALL ME ISHMAEL')
  expect(link).exists()

This test ALSO passes! ❌

This is because .findBy... returns a Promise, which will always be .ok() and will always .exist()

Suggestion: rewrite tests to use .getBy... or .queryBy... instead. Or use await .findBy... where necessary. Example, for FinishedAnnouncementConnector.spec.js:

  const link = screen.getByRole('link')
  expect(link?.getAttribute('href')).to.equal('/projects/zookeeper/galaxy-zoo/about/results')

Dev Notes

This is a list of all `*.spec.js* files that use .findBy, as of 14 Oct 2024. I've linked to the _current_ (master) version instead of 4062f39 so you can quickly check if they're already patched.

app-project

• ❌ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/components/Announcements/components/FinishedAnnouncement/FinishedAnnouncementConnector.spec.js
• ❌ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/components/Announcements/components/GenericAnnouncement/GenericAnnouncement.spec.js
• ❌ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/components/Announcements/components/ProjectAnnouncement/ProjectAnnouncementConnector.spec.js
• ❌ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/components/PageHeader/PageHeader.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/components/ProjectHeader/components/DropdownNav/DropdownNav.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/components/ProjectHeader/components/LocaleSwitcher/LocaleSwitcher.spec.js
• ❌ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/screens/ClassifyPage/components/YourStats/YourStats.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/app-project/src/shared/components/CollectionsModal/components/SelectCollection/SelectCollection.spec.js

lib-classifier

• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/components/Classifier/Classifier.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/components/Classifier/ClassifierContainer.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/components/TranscribedLines/TranscribedLines.spec.js
• ❌ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/plugins/tasks/survey/components/SurveyTask.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/plugins/tasks/survey/components/SurveyTask.userClicks.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-classifier/src/plugins/tasks/survey/components/SurveyTask.userKeystrokes.spec.js

lib-react-components

• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-react-components/src/Media/Media.spec.js
• mostly ✅ , but one ❌ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-react-components/src/Media/components/ThumbnailImage/ThumbnailImage.spec.js
• ✅ https://github.com/zooniverse/front-end-monorepo/blob/master/packages/lib-react-components/src/ZooHeader/ZooHeader.spec.js

❌ indicates the test isn't working correctly (i.e. giving false 'pass'es), as described in this issue.
✅ indicates that the test actually seems OK, since it uses e.g. await screen.findBy...(), which is the correct way to use Promises. (Not sure if it's worth double-checking.)

Status

Not urgent, but should be assigned on the next maintenance week. The incorrect tests means we're missing some safety nets that we assume are there.

[eatyourgreens]

I’ve fixed one of these in #6374.

[eatyourgreens]

A testing tip: specify the accessible name of a node in role queries, to avoid false positives.

Instead of

  const link = screen.getByRole('link')
  expect(link?.getAttribute('href')).to.equal('/projects/zookeeper/galaxy-zoo/about/results')

look for the link by its name. This also helps catch links that don't have accessible link text.

  const link = screen.getByRole('link', { name: 'See results' })
  expect(link?.getAttribute('href')).to.equal('/projects/zookeeper/galaxy-zoo/about/results')

[eatyourgreens]

#6399 fixes another set of findBy tests that were passing when they should fail.