-
Notifications
You must be signed in to change notification settings - Fork 138
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.js
files incompare.js
- Remove existing
*.page-vue-render.js
files from test expectations - Add
*.page-vue-render.js
to.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 |
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.js
function to skip comparison check for PageVueRenderFiles.page-vue-render.js
files from inexpected
snapshots.page-vue-render.js
to gitignores to prevent it from being commited during futureupdatetest
Will 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.js
files 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):