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.

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

Jump to bottom

[Search]: Optimize search bar test suite #18375

Merged
merged 2 commits into from
Jun 27, 2023
Merged

[Search]: Optimize search bar test suite #18375

merged 2 commits into from
Jun 27, 2023

Conversation

camilaibs
Copy link
Contributor

@camilaibs camilaibs commented Jun 21, 2023 •
edited
Loading

Hey, I just made a Pull Request!

In this pull request, we are seeking to make the speed of the SearchBar test more consistent and reduce its instability.

Performing the following test changes reduced the SearchBar test run time from 27.501 seconds to 6-9 seconds and could help us to reduce the risk of flakiness:

  • Using only render with effects to wait for state changes during the first render
  • Running all timers after tests using faking timers and setting debounce time to zero in tests that don't use fake timers
  • Returning expects on waitFor calls to avoid unstable tests that sometimes pass in the next tick sometimes not
  • Remove all *byRole queries, because of their performance issues; although it is recommended, this operation is expensive.

Before optimizations

1st run 2nd run
image image

After optimizations

Watch Mode (Local)

1st run 2nd run
image image

In Band Mode (CI)

1st run 2nd run
image image

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@github-actions github-actions bot added area:discoverability Related to the Discoverability Project Area search Things related to Search labels Jun 21, 2023
@camilaibs camilaibs changed the title test(search): optimize search bar suite [Search]: Optimize search bar test suite Jun 21, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 21, 2023 •
edited
Loading

Uffizzi Preview deployment-29120 was deleted.

@camilaibs camilaibs force-pushed the optimize-search-bar-tests branch from 719364b to ba4e2bf Compare June 22, 2023 07:02
camilaibs
camilaibs commented Jun 22, 2023
View reviewed changes
plugins/search-react/src/components/SearchBar/SearchBar.test.tsx Outdated
act(() => {
jest.advanceTimersByTime(200);
});
await waitFor(() => expect(analyticsApiMock.getEvents()).toHaveLength(1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was flaky. Sometimes the value state was updated before the analytics events trigger, so here we inverted the checking, waiting for analytics events to be captured first. That way we know for sure that both value and events were updated.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, you could have one waitFor with two expect's at once in the inner function, to detach yourselves from any sequencing risks

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @freben. I'm taking over this PR since @camilaibs is not available this week.
I'm not sure if I understood you 100%, if added baca931 with a fix attempt , could you take a look?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's what I had in mind, one waitFor with several expect inside.

I'm out for vacations now though!

freben
freben reviewed Jun 22, 2023
View reviewed changes
Copy link
Member

@freben freben left a comment

Choose a reason for hiding this comment

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

Sweet!

plugins/search-react/src/components/SearchBar/SearchBar.test.tsx Outdated
act(() => {
jest.advanceTimersByTime(200);
});
await waitFor(() => expect(analyticsApiMock.getEvents()).toHaveLength(1));
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, you could have one waitFor with two expect's at once in the inner function, to detach yourselves from any sequencing risks

plugins/search-react/src/components/SearchBar/SearchBar.test.tsx
</SearchContextProvider>
</TestApiProvider>,
);

const textbox = await screen.findByRole('textbox', { name: 'Search' });
const textbox = screen.getByLabelText('Search');
Copy link
Member

Choose a reason for hiding this comment

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

After fixing the other condition, does avoiding findByRole still make a measurable change in runtime? This one was a surprise for me for sure!

Copy link
Contributor

Choose a reason for hiding this comment

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

I've ran the tests again with both options but didn't find a clear and relevant difference between both. I'm using yarn test plugins/search-react/src/components/SearchBar/SearchBar and changing between both implementations.
But I see no reason to change it back. Do you have any worries keeping this line?

Copy link
Member

Choose a reason for hiding this comment

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

No, no worries. Just use what you feel is the clearest

@aitherios aitherios force-pushed the optimize-search-bar-tests branch from ba4e2bf to baca931 Compare June 27, 2023 08:45
@github-actions github-actions bot added area:catalog Related to the Catalog Project Area area:permission Related to the Permission Project Area area:techdocs Related to the TechDocs Project Area auth documentation Improvements or additions to documentation homepage Features for the composable homepage microsite Changes to backstage.io area:scaffolder Everything and all things related to the scaffolder project area labels Jun 27, 2023
@backstage-goalie
Copy link
Contributor

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

camilaibs and others added 2 commits June 27, 2023 13:34
@camilaibs @aitherios
test(search): optimize search bar suite
716b920 
Signed-off-by: Camila Belo <camilaibs@gmail.com>
@aitherios
test(search): Move expectations inside waitFor
954a7d5 
Signed-off-by: Renan Mendes Carvalho <aitherios@gmail.com>
@aitherios aitherios force-pushed the optimize-search-bar-tests branch from baca931 to 954a7d5 Compare June 27, 2023 11:34
@github-actions github-actions bot removed area:techdocs Related to the TechDocs Project Area area:scaffolder Everything and all things related to the scaffolder project area area:permission Related to the Permission Project Area area:catalog Related to the Catalog Project Area microsite Changes to backstage.io auth homepage Features for the composable homepage documentation Improvements or additions to documentation labels Jun 27, 2023
@aitherios aitherios marked this pull request as ready for review June 27, 2023 11:56
@aitherios aitherios requested a review from a team as a code owner June 27, 2023 11:56
@aitherios aitherios merged commit dd7dfaa into master Jun 27, 2023
@aitherios aitherios deleted the optimize-search-bar-tests branch June 27, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freben freben freben left review comments

@aitherios aitherios aitherios approved these changes

Assignees
No one assigned
Labels
area:discoverability Related to the Discoverability Project Area search Things related to Search
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@camilaibs @aitherios @freben