Classifier: Refactor survey task tests

[mcbouslog]

Package

• lib-classifier

Describe your changes

• refactor survey task tests for when choice with required questions selected
• refactor some (not all, left at least one test with getByRole for related component) tests with getByText instead of getByRole
• improves SurveyTask.spec.js tests runtime by approximately 4 seconds

How to Review

Helpful explanations that will make your reviewer happy:

• What Zooniverse project should my reviewer use to review UX?
• What user actions should my reviewer step through to review this PR?
• Which storybook stories should be reviewed?
• Are there plans for follow up PR’s to further fix this bug or develop this feature?

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

• Tests are passing locally and on Github
• Documentation is up to date and changelog has been updated if appropriate
• You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
• FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
• FEM works in a mobile browser

Refactoring

• The PR creator has described the reason for refactoring
• The refactored component(s) continue to work as expected

[coveralls]

Coverage increased (+0.3%) to 82.478% when pulling 158d3be on survey-task-tests-refactor into 103fc15 on master.

[eatyourgreens]

Is there a reason that you’re not using screen to run queries?

Buttons and other labelled elements should use getByLabelText rather than getByText.

[eatyourgreens]

A couple of other thoughts:

• within can help to narrow the scope of a query eg. only look for menu items inside a menu. That could speed queries up.
• you also have fast APIs like querySelector and querySelectorAll to find DOM nodes for testing.
• you can use beforeEach to reduce repetition in test blocks.
• caching query results with a variable is faster than rebuilding the DOM and finding them again, when you want to test multiple elements on the same screen.

[eatyourgreens]

Don’t use ARIA labels to duplicate button labels.

[eatyourgreens]

Why has the Identify button label been changed to 'Identify Aardark', 'Identify Fire' etc? It should be 'Identify' for any choice.

[mcbouslog]

lib-react-components and Grommet buttons (Primary, Button, etc.) that define the label as a property on the component are not found with getByLabelText. If the noted buttons have an a11yTitle they are found with getByLabelText.

Components that are found with getByLabelText:

• ChoiceButton
• FilterStatus

Components with defined labels that aren't found with getByLabelText:

• ClearFilters
• (within Choice) Not this
• (within Choice) Identify

[eatyourgreens]

Is that because you're looking for eg. SurveyTask.Choice.identify but the label is 'Identify'?

[eatyourgreens]

No wait, the docs say this:

The example above does NOT find the input node for label text broken up by elements.

https://testing-library.com/docs/queries/bylabeltext#name

So those buttons could be failing because they use HTML markup in their labels. ChoiceButton here, for example, has markup in its label, because it uses images.

ChoiceButton there, by the way, should not have an ARIA label. A button should either have a text label:

<button>Text label</button>

or an ARIA label:

<button aria-label="Text label"><img /></button>

but never both together.

To be honest, in that second case the button label should be image alt text.

<button><img alt="Text label" /></button>

[mcbouslog]

For buttons with HTML markup in the labels, preventing use of getByLabelText, what do you think is the preferred query? I think ideally it'd be getByRole('button', { name: 'Aardvark' }), but that query takes awhile, so maybe for these tests getByText?

[eatyourgreens]

This query will find all menu item checkboxes inside the first menu on the page. It should be fast too.

querySelector([role=“menu”]).querySelectorAll([role=“menuitemcheckbox”])

If the checkboxes have values, you can further refine the query with [value=“Aardvark”]. Otherwise, you could pluck the first item from the returned NodeList, or filter the node list by inspecting the text of each element to find a particular button.

Stepping back from specific tests, is the run time being spent rendering the survey task, or running queries? If it’s rendering excessively, you can cache rendered nodes in variables, as snapshots of the DOM state, and reuse them.

eg.

const choicesMenu = document.querySelector([role=“menu”])

const choiceButtons = choiceMenu.querySelectorAll([role=“menuitemcheckbox”])

/*
 Reuse choiceMenu and choiceButtons
 to save time later. 
*/

const aardvarkButton = Array.from(choiceButtons).find(button => button.textContent === ‘Aardvark’)

Have a look at the Classifier tests, which do this because they are also slow. The Classifier tests are also split into two files, to speed them up a bit, so consider that too.

[eatyourgreens]

Reading the source code, label text queries ignore buttons.
https://github.com/testing-library/dom-testing-library/blob/edffb7c5ec2e4afd7f6bedf842c669ddbfb73297/src/queries/label-text.ts#L25

[eatyourgreens]

Also noting here that ChoiceButton, linked above, wraps a div with button, which is invalid HTML.

[eatyourgreens]

This is running a lot faster now on my laptop, though the excessive rendering is still making it quite slow.

I've rebased and added a commit that renders a story once, in a before() block, then tests the output DOM of that story in separate it() blocks. You can use that as an example to speed up the other test blocks. There are a lot of tests where you run the same render multiple times. You might be able to run it once, store the output in variables then test each of those variables in it() blocks. I think the only places where you do need to re-render from scratch are tests where you're running a specific sequence of interactions inside an it(), changing the state of the component and testing that as you go.

I also fixed a bug in the 'Clear FIlters' button, which had the wrong label text. Your tests had caught the bug, but you'd updated the tests to pass by using the broken label, rather than fixing the button so that it passed with the expected label.

[eatyourgreens]

CI isn't timing out, so I'm happy for this to be merged as-is.

[eatyourgreens]

I still can't figure out how to remove a filter, in the storybook, using only the keyboard. Definitely check that the user interactions in the keyboard tests match what a real volunteer would be expected to do. I have a feeling we're baking bad UX into the tests, which means they'll fail when the UX failure is fixed.

[mcbouslog]

Thank you @eatyourgreens ! I'll merge this shortly.

For SurveyTask.spec.js,

• before these changes:

• with these changes:

Adding data-testid to the identify button and reducing the tabs to select a required choice question answer cut the related longest running test from 1661ms to 513ms for me locally.

The filters can be removed by tabbing to the close/remove button within the filter and pressing space or enter. I can record a video and share on Slack if it'd be helpful. Regardless, I think the filters need a substantive refactor as noted in #3919. I'm about to add a comment in that issue to further discuss.

[eatyourgreens]

The filters can be removed by tabbing to the close/remove button within the filter and pressing space or enter.

This is a bug. See #3919. Tab on each radio button should take you out of the radio button group and on to the next focusable element on the page.

[eatyourgreens]

FWIW, I'm toggling the selected radio button with the cursor keys, then trying Space or Enter to clear the selected value.