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

chore: Drill to detail Modal tests #21148

Merged
merged 14 commits into from
Aug 29, 2022
Merged

Conversation

geido
Copy link
Member

@geido geido commented Aug 22, 2022

SUMMARY

This PR adds RTL tests for the drill to detail modal originally implemented in this PR #20728

Related E2E tests: #21187

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N.A.

TESTING INSTRUCTIONS

All the tests should succeed

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #21148 (c42fb64) into master (5f76ac9) will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21148      +/-   ##
==========================================
+ Coverage   66.40%   66.51%   +0.10%     
==========================================
  Files        1783     1783              
  Lines       68087    68087              
  Branches     7261     7261              
==========================================
+ Hits        45215    45288      +73     
+ Misses      21007    20931      -76     
- Partials     1865     1868       +3     
Flag Coverage Δ
hive 53.11% <ø> (ø)
javascript 52.50% <ø> (+0.20%) ⬆️
mysql 81.00% <ø> (ø)
postgres 81.05% <ø> (ø)
presto 53.01% <ø> (ø)
python 81.53% <ø> (ø)
sqlite 79.57% <ø> (ø)
unit 50.67% <ø> (ø)

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

Impacted Files Coverage Δ
...dashboard/components/SliceHeaderControls/index.tsx 70.21% <0.00%> (+1.06%) ⬆️
...erset-frontend/src/components/Chart/chartAction.js 56.77% <0.00%> (+1.29%) ⬆️
...frontend/src/components/Chart/DrillDetailModal.tsx 100.00% <0.00%> (+17.64%) ⬆️
.../src/dashboard/components/DrillDetailPane/utils.ts 57.14% <0.00%> (+57.14%) ⬆️
...ard/components/DrillDetailPane/DrillDetailPane.tsx 82.75% <0.00%> (+81.03%) ⬆️
...board/components/DrillDetailPane/TableControls.tsx 94.11% <0.00%> (+94.11%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@geido geido marked this pull request as ready for review August 24, 2022 18:20
@github-actions
Copy link
Contributor

Storybook has completed and can be viewed at

});
};
const waitForRender = (overrides: Record<string, any> = {}) =>
waitFor(() => setup(overrides));
Copy link
Member

Choose a reason for hiding this comment

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

Is it really needed? Don't we always wait for render by default?

Copy link
Member Author

@geido geido Aug 25, 2022

Choose a reason for hiding this comment

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

@kgabryje in this component there are several async events that would fire warnings during the test runs if we don't wait for the component to be fully loaded. Hence the reason for this.

Copy link
Member

Choose a reason for hiding this comment

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

Oh that's interesting. Does it mean that we should use this pattern in other components too? Maybe this should be a helper function in spec/helpers?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kgabryje not sure if it is worth creating a function that takes a component and wraps it in a waitFor while also taking eventual overrides but if you think it can help I am happy to create it in a separate PR.

Copy link
Member Author

@geido geido Aug 25, 2022

Choose a reason for hiding this comment

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

In general, I think there are a lot of reusable helpers that we can create for these RTL tests. This could be an interesting project in order to enforce/suggest certain standards in writing RTL tests. CC @rusackas

Copy link
Member

Choose a reason for hiding this comment

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

Ok, approving this PR 👍 Do you think though that other tests would benefit from using this pattern? Should we use it to decrease the number of warnings?

Copy link
Member

Choose a reason for hiding this comment

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

You can also use findBy in many of these tests if you want to wait for the async operations. findBy is essentially the combination of getBy and waitFor. So this:

test('should render the title', async () => {
  await waitForRender();
  expect(
    screen.getByText(`Drill to detail: ${chart.form_data.slice_name}`),
  ).toBeInTheDocument();
});

Could be written as:

test('should render the title', async () => {
  setup();
  expect(
    await screen.findByText(`Drill to detail: ${chart.form_data.slice_name}`),
  ).toBeInTheDocument();
});

Either way, the important thing is to account for the async nature of the component as you did.

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

geido and others added 5 commits August 26, 2022 14:16
…leControls.test.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
…leControls.test.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
…leControls.test.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
…leControls.test.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
…leControls.test.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
@github-actions
Copy link
Contributor

Storybook has completed and can be viewed at

4 similar comments
@github-actions
Copy link
Contributor

Storybook has completed and can be viewed at

@github-actions
Copy link
Contributor

Storybook has completed and can be viewed at

@github-actions
Copy link
Contributor

Storybook has completed and can be viewed at

@github-actions
Copy link
Contributor

Storybook has completed and can be viewed at

@geido geido merged commit 1d3a805 into master Aug 29, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
@mistercrunch mistercrunch deleted the chore/drill-to-detail-modal-tests branch March 26, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants