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: verify changes on allowedFiles according to the template #1048

Merged

Conversation

Aslemammad
Copy link
Contributor

script/migrate-test-e2e.js

PR Checklist

Overview

We track the changes using createRootFiles and we watch the changes files based on the function result.

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2f8e90f) 94.29% compared to head (838340b) 94.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1048      +/-   ##
==========================================
- Coverage   94.29%   94.27%   -0.02%     
==========================================
  Files          97       97              
  Lines        5626     5626              
  Branches      455      455              
==========================================
- Hits         5305     5304       -1     
- Misses        320      321       +1     
  Partials        1        1              
Flag Coverage Δ
create 70.22% <ø> (ø)
initialize 42.16% <ø> (ø)
migrate 69.60% <ø> (-0.08%) ⬇️
unit 72.32% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

A good start, thanks for looking into this! I think the general strategy is mostly implemented well... but the strategy itself I think is going to need to be reworked. What do you think?

script/migrate-test-e2e.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Nov 21, 2023
@JoshuaKGoldberg
Copy link
Owner

JoshuaKGoldberg commented Nov 21, 2023

Btw, looks like there are some CI failures. You'll want to fix them up. .github/DEVELOPMENT.md should be helpful in pointing out the tools. If anything's unclear let me know!

@JoshuaKGoldberg
Copy link
Owner

Whoops, I pushed to the wrong branch working on another PR - sorry about that :)

@Aslemammad Aslemammad force-pushed the fix/verify-changes-in-hydration branch from b7878d9 to 8b92ae4 Compare November 27, 2023 06:43
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Progress! 🚀

script/migrate-test-e2e.js Outdated Show resolved Hide resolved
script/verify-changes.test.ts Outdated Show resolved Hide resolved
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🙌 this is looking great, nice work! I like the snapshot approach. And it's already found a bunch of discrepancies - filed inline to fix separately. Yay!

Requesting changes largely because the script never finishes running, and filtering out a few specific things from the snapshots. I left a few questions inline too.

script/__snapshots__/verify-changes.test.ts.snap Outdated Show resolved Hide resolved
script/__snapshots__/verify-changes.test.ts.snap Outdated Show resolved Hide resolved
script/__snapshots__/verify-changes.test.ts.snap Outdated Show resolved Hide resolved
script/__snapshots__/verify-changes.test.ts.snap Outdated Show resolved Hide resolved
script/__snapshots__/verify-changes.test.ts.snap Outdated Show resolved Hide resolved
script/__snapshots__/verify-changes.test.ts.snap Outdated Show resolved Hide resolved
script/__snapshots__/verify-changes.test.ts.snap Outdated Show resolved Hide resolved
script/verify-changes.test.ts Outdated Show resolved Hide resolved
script/migrate-test-e2e.js Outdated Show resolved Hide resolved
script/migrate-test-e2e.js Show resolved Hide resolved
@Aslemammad
Copy link
Contributor Author

@JoshuaKGoldberg Let me know if this is ready for a merge.

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Wonderful, thanks so much for all your hard work on this PR @Aslemammad! I think it looks great and will be very helpful keeping the last few files aligned to the templates as time goes on.

I'll apply a couple touchups myself for linking to upstream bugs & adding back bin/index.js. But no more blockers. Yay!

Guy with mustache and mullet excitedly saying 'yes that is awesome'

script/migrate-test-e2e.js Show resolved Hide resolved
Comment on lines 842 to 843
- "packageManager": "pnpm@8.12.1",
+ "packageManager": "pnpm@8.7.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Heh, and in CI:

2023-12-30T06:48:46.9914884Z    	},
2023-12-30T06:48:46.9915527Z - -	"packageManager": "pnpm@8.12.1",
2023-12-30T06:48:46.9916260Z + -	"packageManager": "pnpm@8.13.1",
2023-12-30T06:48:46.9916878Z   +	"packageManager": "pnpm@8.7.0",

This'll probably start failing as soon as a new pnpm version is out. Lovely. That can be a followup.

@Aslemammad
Copy link
Contributor Author

I'll apply a couple touchups myself for linking to upstream bugs & adding back bin/index.js. But no more blockers. Yay!

Thank you so much, happy to see it finally being merged!

@JoshuaKGoldberg
Copy link
Owner

❌ codecov/project — 94.27% (-0.02%) compared to 2f8e90f

...very amusing.

@JoshuaKGoldberg JoshuaKGoldberg changed the title feat: verify changes on allowedFiles according to the template chore: verify changes on allowedFiles according to the template Dec 30, 2023
@JoshuaKGoldberg
Copy link
Owner

Oh, and: renaming the PR title to chore: because this is just an internal-only improvement. It doesn't impact end-users.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 7390c17 into JoshuaKGoldberg:main Dec 30, 2023
17 of 18 checks passed
@JoshuaKGoldberg
Copy link
Owner

@all-contributors please add @Aslemammad for code.

🤖 Beep boop! This comment was added automatically by all-contributors-auto-action.
Not all contributions can be detected from Git & GitHub alone. Please comment any missing contribution types this bot missed.
...and of course, thank you for contributing! 💙

Copy link
Contributor

@JoshuaKGoldberg

I've put up a pull request to add @Aslemammad! 🎉

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

JoshuaKGoldberg added a commit that referenced this pull request Dec 30, 2023
Adds @Aslemammad as a contributor for code.

This was requested by JoshuaKGoldberg [in this
comment](#1048 (comment))

---------

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>
Copy link

🎉 This is included in version v1.50.2 🎉

The release is available on:

Cheers! 📦🚀

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.

🧪 Testing: End-to-end hydrate tests should still verify expected changes to files are correct
2 participants