-
Notifications
You must be signed in to change notification settings - Fork 14
Adds integrity check of formatter as a CI/CD step #416
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
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
bc6d2a9
add sample queries
tomasnyberg 2693ab9
move sample_queries.json to tests
tomasnyberg 2388b8f
move the sample queries.json file (again...)
tomasnyberg 899c55e
add file that runs all queries and throws on errorgs
tomasnyberg 20bbad4
add the github action, seems to be working (not without the java thin…
tomasnyberg 17edd03
clean up ci.yaml (remove comment, blank line)
tomasnyberg 0d9d61e
better name for the verification check gsjob itself
tomasnyberg 1c668c0
format verificationCheck.ts
tomasnyberg 0f8f735
refactor verificaitonCheck a bit
tomasnyberg 9d8f93e
correct name for job
tomasnyberg 58f2aa6
introduce dumb change to check that it fails
tomasnyberg ebe9508
try to get check to run only on certain changes
tomasnyberg 6ab7ec9
remove paths to check if that's the issue
tomasnyberg 5c95913
correct path
tomasnyberg 62ed742
remove commented out visit
tomasnyberg 9fd7150
remove commented out part
tomasnyberg 15eada0
add meaningless change to verify happy path
tomasnyberg 9054ab9
remove menaingless change
tomasnyberg 68d226e
add anonymized comments to the sample queries
tomasnyberg 4eba9da
rename ciformatting.yaml -> formatting-integrity-check.yaml
tomasnyberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| paths: | ||
| - 'packages/language-support/src/formatting/**' | ||
|
|
||
| pull_request: | ||
| branches: | ||
| - main | ||
| paths: | ||
| - 'packages/language-support/src/formatting/**' | ||
|
|
||
| jobs: | ||
| formatting-integrity-check: | ||
| name: Formatter integrity check | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup antlr4 | ||
| uses: ./.github/actions/setup-antlr4 | ||
|
|
||
| - name: Install dependencies with frozen lock file and generate parser | ||
| run: npm ci | ||
|
|
||
| - name: Build Project | ||
| run: npm run build | ||
|
|
||
| - name: Run formatting check | ||
| run: npm run test:formattingIntegrity |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11,935 changes: 11,935 additions & 0 deletions
11,935
packages/language-support/src/tests/formatting/verification/sample_queries.json
Large diffs are not rendered by default.
Oops, something went wrong.
53 changes: 53 additions & 0 deletions
53
packages/language-support/src/tests/formatting/verification/verificationCheck.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
| import { formatQuery } from '../../../formatting/formatting'; | ||
| import { standardizeQuery } from '../../../formatting/standardizer'; | ||
|
|
||
| function throwError(message: string, query: string, formatted: string): never { | ||
| throw new Error(`${message}, | ||
| --------- QUERY BEFORE START ------------ | ||
| ${query} | ||
| --------- QUERY BEFORE END ---------- | ||
| --------- QUERY FORMATTED START ------------ | ||
| ${formatted} | ||
| --------- QUERY FORMATTED END ---------- | ||
|
Comment on lines
+8
to
+14
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This leaves some pretty nice output which should make it easy to debug, see this one for instance: https://github.com/neo4j/cypher-language-support/actions/runs/13951159540/job/39050765538 |
||
| `); | ||
| } | ||
|
|
||
| function verifyFormatting(query: string): void { | ||
| const formatted = formatQuery(query); | ||
| const queryStandardized = standardizeQuery(query); | ||
| const formattedStandardized = standardizeQuery(formatted); | ||
| const originalNonWhitespaceCount = query.replace(/\s/g, '').length; | ||
| const formattedNonWhitespaceCount = formatted.replace(/\s/g, '').length; | ||
|
|
||
| // Non-whitespace character count check | ||
| if (originalNonWhitespaceCount !== formattedNonWhitespaceCount) { | ||
| throwError('Non-whitespace character count mismatch', query, formatted); | ||
| } | ||
|
|
||
| // AST integrity check | ||
| if (formattedStandardized !== queryStandardized) { | ||
| throwError( | ||
| 'Standardized query does not match standardized formatted query', | ||
| query, | ||
| formatted, | ||
| ); | ||
| } | ||
|
|
||
| // Idempotency check | ||
| const formattedTwice = formatQuery(formatted); | ||
| if (formattedTwice !== formatted) { | ||
| throwError('Formatting is not idempotent', query, formatted); | ||
| } | ||
| } | ||
|
|
||
| function verifyFormattingOfSampleQueries() { | ||
| const filePath = path.join(__dirname, 'sample_queries.json'); | ||
| const fileContent = fs.readFileSync(filePath, 'utf-8'); | ||
| const queries: string[] = JSON.parse(fileContent) as string[]; | ||
| queries.forEach((query) => verifyFormatting(query)); | ||
| } | ||
|
|
||
| verifyFormattingOfSampleQueries(); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not sure if there's a more idiomatic way to run a file as a script? Or maybe it shouldn't be run as a script at all, maybe there's a better way? Happy to take any suggestions here.
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.
In other cases, I've made the script build js and then execute it. We've also needed to use
cross-envfor some cases (see thegen-parserscript, although being on mac I don't really know why it's needed 😅 )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.
For the record I think it's fine to leave as is here
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.
cross-envis only to use environment variables (we cannot set them for builds on Windows otherwise)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.
If we move to
node22we can run it without ts-node as well with--experimental-strip-types