-
Notifications
You must be signed in to change notification settings - Fork 140
Gitignore page-vue-render.js files in CLI Functional Tests #2718
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
Gitignore page-vue-render.js files in CLI Functional Tests #2718
Conversation
*.page-vue-render.js files that exist in expected snapshots for CLI functional tests have diffs that are unreadable. Remove diff checking for these files, and added gitignore to prevent these files from being unnecessarily committed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2718 +/- ##
========================================
Coverage 52.84% 52.84%
========================================
Files 130 130
Lines 7162 7162
Branches 1572 1533 -39
========================================
Hits 3785 3785
+ Misses 3222 3072 -150
- Partials 155 305 +150 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR streamlines CLI functional tests by removing and ignoring verbose Vue render snapshot files.
- Skip comparing
*.page-vue-render.jsfiles incompare.js - Remove existing
*.page-vue-render.jsfiles from test expectations - Add
*.page-vue-render.jsto.gitignore
Reviewed Changes
Copilot reviewed 111 out of 111 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/test/functional/test_site/expected/testGlyphiconInPage.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testFontAwesomeInPage.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testExternalScripts.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testEmptyFrontmatter.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testEmptyAltFrontMatter.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testDates.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testCodeBlocks.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testCenterText.page-vue-render.js | Cleared out verbose Vue render snapshot file |
| packages/cli/test/functional/test_site/expected/testBootstrapIconInPage.page-vue-render.js | Cleared out verbose Vue render snapshot file |
Comments suppressed due to low confidence (1)
packages/cli/test/functional/test_site/expected/testGlyphiconInPage.page-vue-render.js:1
- [nitpick] Since the file has been emptied, consider deleting the file entirely instead of leaving an empty placeholder; this makes the test directory clearer.
-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a minor issue with the current fix. Although *.page-vue-render.js files are correctly ignored by .gitignore and not committed, they are still generated locally whenever npm run updatetest is ran with changes to the code.
This causes npm run test to throw the following error:

The issue arises because the test comparison logic filters out *.page-vue-render.js from the actual output but not from the expected snapshot paths, which may now include these newly generated files. We can fix this by also filtering out *.page-vue-render.js from expectedPaths in case of these newly generated files.
Alternatively, we can try to prevent these files from being generated in the expected snapshot paths, but I'm not familiar with how feasible or practical this solution will be.
EDIT:
Actually, is it possible to just add *.page-vue-render.js to the TEST_BLACKLIST instead of filtering out the files specifically? That way, the files existence is still checked but the content itself is ignored so there wouldn't be any time incurred from diff.
|
Thanks for spotting what I missed out! Indeed, on updating tests, the render files will be there and cause an error, even if not commited and gitignored. I tested your suggested change and it works well, by excluding these render files that are generated on test update!
Checking file existence could be beneficial, but my rationale for completely removing the render files existence was to lower version history bloat for these render files, as well as reduce changes shown on PR diffs when updating tests. What do you think> Accidentally clicked close this branch |
AgentHagu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This change skips any diff checks for *.page-vue-render.js files, reducing overhead during tests.
I see! In that case, I think the current solution should work best for us |
|
@gerteck Each PR must have a SEMVER impact label, please remember to label the PR properly. |
What is the purpose of this pull request?
Overview of changes:
Fixes #2623
Partially Addresses #1637 (See last bullet point)
This PR does basically 3 things.
compare.jsfunction to skip comparison check for PageVueRenderFiles.page-vue-render.jsfiles from inexpectedsnapshots.page-vue-render.jsto gitignores to prevent it from being commited during futureupdatetestWill need help to verify correctness of changes and no unrelated changes or regressions. Thank you!
Anything you'd like to highlight/discuss:
The
*.page-vue-render.jsfiles that exist in expected snapshots for CLI functional tests have diffs that are unreadable.This PR removes diff checking for these files (in
compare.js), adds gitignore to prevent these files from being unnecessarily committed during snapshot updates and removes the associated render files from the repository.Testing instructions:
Automated tests should work and pass, in particular, CLI functional tests.
Proposed commit message: (wrap lines at 72 characters)
Remove page-vue-render.js files from test
*.page-vue-render.js files that exist in expected snapshots for CLI
functional tests have diffs that are unreadable.
Remove diff checking for these files, and add gitignore to
prevent files from being unnecessarily committed.
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):