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

Upgrade to PostCSS 8 #127

Merged
merged 2 commits into from
Feb 5, 2021
Merged

Upgrade to PostCSS 8 #127

merged 2 commits into from
Feb 5, 2021

Conversation

onigoetz
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Upgrade to PostCSS 8

Please Describe Your Changes

Hello,

This PR upgrades to PostCSS 8,
I had a look at the updated snapshots and they seem fine.

However A change made in PostCSS ( postcss/postcss@0687620 ) Makes the snapshots non-reproducible.

I already open the PR because I'm not sure what course of action we should take as I'm not familiar with AVA.

My ideas would be

  • patch the snapshot with a Regex and replace the non-unique IDs with unique ones (although there seems to be no possibility of setting options or serializers for snapshots in AVA)
  • mock the ID generator to always return the same ID
  • generate temp files, since the IDs are generated only with non named inputs, but that might still be the same problem if the whole file path is added to the output

I'm willing to pass that PR through the finish line but I'm not sure what option to take or if they're even possible

@shellscape
Copy link
Owner

Thanks for working on this 👍

We can pass the --u flag to the test command to update the snapshots. We couldn't patch them manually because they have an accompanying binary file for fidelity and fast comparison. If all of the data minus the IDs are correct, then the snapshots can be safely updated.

@onigoetz
Copy link
Contributor Author

That's the thing, I did update the snapshots.
But since they use a randomly generated IDs, they are different on each run.
That and PostCSS's AST changed (For example adding an "offset" field).

My idea was not to update the snapshots on disk manually, but to pass the result through a filter and then compare it.
I do that in another project where I test a build tool and it displays how long it took to build, I replaced "25ms" with "__ms" since that's not a relevant information for tests

@shellscape
Copy link
Owner

Ah, I see now. Yeah that approach should be fine

@onigoetz
Copy link
Contributor Author

Okay, so I went with a different approach : used rewiremock to mock the nanoid module and replace it with an implementation that's not random.

Tests are now running smoothly

@onigoetz
Copy link
Contributor Author

@shellscape Hello, do you need anything from me on this PR ?

@shellscape shellscape mentioned this pull request Nov 3, 2020
@shellscape
Copy link
Owner

Thank you for putting this together, and I'm sorry it took me so very long to get around to merging this. I really appreciate your efforts!

@shellscape shellscape merged commit 5cd85c7 into shellscape:master Feb 5, 2021
@onigoetz
Copy link
Contributor Author

onigoetz commented Feb 5, 2021

Thanks for merging! No worries life happens outside GitHub as well :D

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.

2 participants