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

[core] Unify tests #4368

Merged
merged 3 commits into from
Apr 11, 2022
Merged

[core] Unify tests #4368

merged 3 commits into from
Apr 11, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Apr 5, 2022 •
edited
Loading

  • Name all the describe focusing on a prop props: propName (we had both props: propName and prop: propName)
  • Remove default value to getColumnValues (I find it very unclear that getColumnValues() gets the 1st column values, better be explicit here)
  • Use document.querySelector generic instead of casting the return value

@flaviendelangle flaviendelangle self-assigned this Apr 5, 2022
@flaviendelangle flaviendelangle added test core Infrastructure work going on behind the scenes labels Apr 5, 2022
@flaviendelangle flaviendelangle changed the title [core] Unify test [core] Unify tests Apr 5, 2022
@mui-bot
Copy link

mui-bot commented Apr 5, 2022 •
edited
Loading

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 258.7 481.1 422.2 383.86 92.239
Sort 100k rows ms 565 1,096.1 764.1 830.96 172.304
Select 100k rows ms 146 303.4 168 193.84 56.806
Deselect 100k rows ms 114 205.8 173.2 161.76 39.187

Generated by 🚫 dangerJS against c41ec91

m4theushw
m4theushw reviewed Apr 5, 2022
View reviewed changes
packages/grid/x-data-grid-pro/src/tests/rowGrouping.DataGridPro.test.tsx Outdated
@@ -104,7 +104,7 @@ describe('<DataGridPro /> - Group Rows By Column', () => {
});
});

describe('prop: rowGroupingModel', () => {
describe('props: rowGroupingModel', () => {
Copy link
Member

Choose a reason for hiding this comment

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

The core repo uses prop: propName: https://github.com/mui/material-ui/blob/32fd4a3249fb1de139b17ed54bd54802b3d50f44/packages/mui-material/src/Autocomplete/Autocomplete.test.js#L112

Copy link
Member Author

Choose a reason for hiding this comment

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

I can put prop: propName to be consistent with the core then

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@flaviendelangle flaviendelangle merged commit 6f91713 into mui:master Apr 11, 2022
@flaviendelangle flaviendelangle deleted the test-props-describe branch April 11, 2022 11:04
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
@flaviendelangle @alexfauquette
[core] Unify tests (mui#4368)
8fa0c1d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m4theushw m4theushw m4theushw approved these changes

Assignees

@flaviendelangle flaviendelangle

Labels
core Infrastructure work going on behind the scenes test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flaviendelangle @mui-bot @m4theushw