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

Improve the performance of the marketplace tests #14935

Merged
merged 6 commits into from
Nov 21, 2022

Conversation

remidej
Copy link
Contributor

@remidej remidej commented Nov 18, 2022

What does it do?

Improve the performance of the marketplace tests:

Before After
index.test.js 13.5s 5.4s
providers.test.js 48.2s 34.9s
plugins.test.js 75.9s 66.2s
Total 77.1s 66.8s

Render the provider tab directly

For provider tests, before every test, the app would render on the default plugins tab, wait for it to load, find the providers tab, click on it and wait to load again. I added a query param so that it renders the right tab directly and only loads once. It saved 10 seconds.

Remove the artificial msw delay

It turns out that even without a delay, the 1st render will still be a loading screen if the request is not cached by react-query. So I removed these

Speed up waitForReload

Mark identified that getByRole was slow, so I removed it there, since that function is called many times in the tests

Use findBy queries

When we want to check something on screen after the application reloads, I found that it was slightly faster to use findBy rather than the combination of waitForReload and getBy. I assume it's because the latter requires selecting the page header in addition to what we actually want.

The exception I found was byRole queries. Since using findBy, the queries get re-ran until they find something, having a slow query in there makes things much worse. So I never used findByRole but instead a combination of waitForReload and getByRole, so that byRole only runs once.

Only keep one snapshot test

We had snapshots in index.test.js, plugins.test.ts and providers.test.js. As suggested by Mark I only kept the index one as the others seem redundant

Use within to scope queries

I don't know if this actually makes a difference in terms of perf. But within seems to be the preferred way to query things inside a particular element. Also it avoids having to import all the selectors at the top of the files.

What this doesn't do

At first I wanted to also stop clearing the react-query cache between each test, but it's actually recommended by the library author.

I had also found comments saying that passing { hidden: false } would make byRole faster. In my testing it was actually slower.

@markkaylor
Copy link
Contributor

markkaylor commented Nov 18, 2022

Nice, I'll take it for a spin on Monday before approving.

For provider tests, before every test, the app would render on the default plugins tab, wait for it to load, find the providers tab, click on it and wait to load again. I added a query param so that it renders the right tab directly and only loads once. It saved 10 seconds.

Awesome! good thinking. Do you think we need the snapshots for plugins and providers? Maybe we could just remove them and take one snapshot in the index.test.js

edit: don't think it makes a difference for performance, but it's a bit redundant isn't it?

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Base: 59.79% // Head: 59.79% // No change to project coverage 👍

Coverage data is based on head (98ff7d8) compared to base (892161e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                   Coverage Diff                    @@
##           market-pagination/pages   #14935   +/-   ##
========================================================
  Coverage                    59.79%   59.79%           
========================================================
  Files                         1340     1340           
  Lines                        32689    32689           
  Branches                      6196     6196           
========================================================
  Hits                         19546    19546           
  Misses                       11296    11296           
  Partials                      1847     1847           
Flag Coverage Δ
front 64.31% <100.00%> (ø)
unit 49.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...in/admin/src/pages/MarketplacePage/tests/server.js 94.59% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@remidej
Copy link
Contributor Author

remidej commented Nov 21, 2022

I agree with you about the snapshots @markkaylor, I added a commit to only keep the one from index.test.js

@markkaylor
Copy link
Contributor

markkaylor commented Nov 21, 2022

I agree with the changes here but the performance seems a bit different on my machine. Actually I'm surprised to see this branch running total time a few seconds slower... That being said the index and providers are definitely running faster 👍

I did jest --clearCache before running each

edit: just realized this PR isn't targeting main

Main
Screenshot 2022-11-21 at 10 06 28

This branch
Screenshot 2022-11-21 at 10 08 27

@remidej remidej merged commit ebdbe0b into market-pagination/pages Nov 21, 2022
@remidej remidej deleted the chore/market-tests-perf branch November 21, 2022 10:33
Copy link
Contributor

@madhurisandbhor madhurisandbhor left a comment

Choose a reason for hiding this comment

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

I am a little late to review this, but this is very great PR 😮, I also learned some things and so tried to suggest some changes.

await user.click(filtersButton);

const categoriesButton = screen.getByTestId('Categories-button');
await user.click(categoriesButton);

const option = screen.getByRole('option', { name: `Custom fields (4)` });
await user.click(option);

await waitForReload();

const optionTag = screen.getByRole('button', { name: 'Custom fields' });
Copy link
Contributor

Choose a reason for hiding this comment

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

const optionTag = screen.getByText('Custom fields');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately that's not precise enough because of the made by strapi icons on the plugin cards

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

3 participants