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

[pigment-css] Improve testing of fixtures #41389

Merged
merged 10 commits into from
Mar 8, 2024

Conversation

brijeshb42
Copy link
Contributor

Now, no need to manually create and write to files. If the output files don't exist, they'll be created with the expected content.
If the contents itself change, use "test:update"
command to automatically update the fixtures.

@brijeshb42 brijeshb42 requested a review from siriwatknp March 7, 2024 06:01
@brijeshb42 brijeshb42 added the package: pigment-css Specific to @pigment-css/* label Mar 7, 2024
@mui-bot
Copy link

mui-bot commented Mar 7, 2024

Netlify deploy preview

https://deploy-preview-41389--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 540871b

@brijeshb42 brijeshb42 force-pushed the pigment/improve-testing branch from 1cc1377 to a6df0ba Compare March 7, 2024 09:41
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

@brijeshb42 I split the pigment.test.ts into features and create a testUtils.ts file to reuse the transformation logic. It lets me run a specific test with it.only and the applying a theme per test.

const CUSTOM_ERROR =
'The file contents have changed. Run "test:update" command to update the file if this is expected.';

describe('Pigment CSS - styled', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the test files are repetitive in code. Only thing that is changing is the file path.
Creating new tests means copying the same content everytime. This was already solved in the previous implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment above that'll handle duplication while still keeping the tests independent.

Copy link
Member

Choose a reason for hiding this comment

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

Creating new tests means copying the same content everytime. This was already solved in the previous implementation.

I'd argue this. The previous implementation does not let me use the test infra APIs like it.only() to run a specific test. Also, a specific theme for each test is not possible.

This made me think of The wrong abstraction. The agree that there are duplications but the tests should also be flexible to add/remove/alter as well.

cc @mnajdova for 3rd opinion.

brijeshb42 and others added 10 commits March 8, 2024 12:45
Now, no need to manually create and write to files.
If the output files don't exist, they'll be created
with the expected content.
If the contents itself change, use "test:update"
command to automatically update the fixtures.
@brijeshb42 brijeshb42 force-pushed the pigment/improve-testing branch from c7b35e5 to 540871b Compare March 8, 2024 07:16
@brijeshb42 brijeshb42 enabled auto-merge (squash) March 8, 2024 07:17
@brijeshb42 brijeshb42 requested a review from siriwatknp March 8, 2024 07:17
@brijeshb42 brijeshb42 merged commit 4e38622 into mui:master Mar 8, 2024
18 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
@oliviertassinari oliviertassinari changed the title [pigment] Improve testing of fixtures [pigment-css]Improve testing of fixtures Mar 10, 2024
@oliviertassinari oliviertassinari changed the title [pigment-css]Improve testing of fixtures [pigment-css] Improve testing of fixtures Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: pigment-css Specific to @pigment-css/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants