Update CI configuration and enhance test coverage reporting#5
Update CI configuration and enhance test coverage reporting#5JoelMiller74 merged 1 commit intomainfrom
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates the CI/CD pipeline and test infrastructure to enhance coverage reporting and test result tracking. The main focus is upgrading to Codecov v5, adding JUnit test results reporting, and fixing a bug in the SQL formatter's column alignment logic. However, there are several critical issues that need to be addressed before merging.
Key Changes:
- Upgraded Codecov action from v4 to v5 and added test results upload capability
- Modified Vitest configuration to output JUnit XML format for test results
- Fixed a bug in the CREATE TABLE column alignment regex to properly handle nested parentheses in column types like VARCHAR(50)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ci.yml | Updated Codecov to v5, added fetch-depth: 2, and integrated test results upload action |
| vitest.config.js | Changed reporter to 'junit', added 'json' to coverage reporters, added outputFile configuration |
| vitest.config.js.map | Reformatted source map from minified to pretty-printed JSON format |
| src/formatter/engine.ts | Improved regex pattern for CREATE TABLE parsing to handle nested parentheses in column type definitions |
| src/test/formatter.vitest.ts | Fixed test expectation (removed extra closing parenthesis) and added debug console.log statement |
| files: ./coverage/lcov.info,./coverage/coverage-final.json | ||
| fail_ci_if_error: true | ||
| verbose: true | ||
| token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
The codecov/test-results-action@v1 expects test results in a supported format (JUnit XML, etc.). However, the action should be configured with the file parameter to specify the location of the test results file.
Add the file parameter:
- name: Upload test results to Codecov
if: ${{ !cancelled() }}
uses: codecov/test-results-action@v1
with:
token: ${{ secrets.CODECOV_TOKEN }}
file: ./test-report.junit.xml| token: ${{ secrets.CODECOV_TOKEN }} | |
| token: ${{ secrets.CODECOV_TOKEN }} | |
| file: ./test-report.junit.xml |
| @@ -268,7 +268,7 @@ export function formatTsql(text: string, { options, config, profile }: EngineCon | |||
| // DDL: align CREATE TABLE column definitions (basic) | |||
| const alignCols = config.get?.('alignColumnDefinitions', true); | |||
| if (alignCols) { | |||
There was a problem hiding this comment.
[nitpick] The regex pattern update improves handling of nested parentheses in column definitions (e.g., VARCHAR(50)), which is good. However, the pattern (?:[^()]*|\([^()]*\))* only handles one level of nesting. If there are column types with multiple nested parentheses (though rare in practice), this pattern won't capture them correctly.
Consider using a more robust approach or add a comment documenting this limitation:
// Note: Supports column types with one level of nested parentheses (e.g., VARCHAR(50))
// Nested function defaults like DEFAULT(GETDATE()) will work, but deeper nesting is not supported
out = out.replace(/CREATE\s+TABLE\s+([\s\S]*?)\s*\(((?:[^()]*|\([^()]*\))*)\)/gi, (m, tbl, cols) => {| if (alignCols) { | |
| if (alignCols) { | |
| // NOTE: The following regex only supports one level of nested parentheses in column definitions. | |
| // For example, it works for types like VARCHAR(50) or defaults like DEFAULT(GETDATE()), | |
| // but will NOT correctly handle deeper nesting such as DEFAULT(SOMEFUNC(FOO(BAR(1)))). | |
| // For full support, a proper SQL parser would be required. |
| const t = 'CREATE TABLE t (id INT, name VARCHAR(50))'; | ||
| const out = formatTsql(t, { options: {} as any, config: cfg({ alignColumnDefinitions: true }), profile: {} }); | ||
| expect(out).toBe('CREATE TABLE t (\n id INT,\n name VARCHAR(50)\n))'); | ||
| console.log('align-cols OUT:\n' + out); |
There was a problem hiding this comment.
Remove this debugging console.log statement before merging. Debug statements should not be committed to the codebase as they can clutter logs and aren't useful in production/CI environments.
| console.log('align-cols OUT:\n' + out); |
| reporter: ['text', 'html'], | ||
| reporter: ['text', 'html', 'json'], | ||
| }, | ||
| outputFile: 'test-report.junit.xml', |
There was a problem hiding this comment.
The vitest.config.js file appears to be out of sync with the source vitest.config.ts file. The TypeScript source file has reporters: ['default'] but the compiled JS shows reporters: ['junit']. This inconsistency suggests the compiled file was manually edited instead of being generated from the TypeScript source.
To fix this:
- Update the vitest.config.ts source file to include the junit reporter and outputFile configuration if these changes are needed
- Recompile to regenerate the .js and .js.map files
The source should be:
reporters: ['junit'],
coverage: {
reporter: ['text', 'html', 'json'],
},
outputFile: 'test-report.junit.xml',| outputFile: 'test-report.junit.xml', |
| reporters: ['junit'], | ||
| coverage: { | ||
| reporter: ['text', 'html'], | ||
| reporter: ['text', 'html', 'json'], | ||
| }, | ||
| outputFile: 'test-report.junit.xml', |
There was a problem hiding this comment.
The outputFile configuration for JUnit reporter requires the reporters array to be an object when using named reporters with options. The current configuration may not work as intended.
Change to:
reporters: [['junit', { outputFile: 'test-report.junit.xml' }]],Or alternatively, if you want both default and junit:
reporters: ['default', ['junit', { outputFile: 'test-report.junit.xml' }]],
outputFile: undefined, // remove this lineSee Vitest documentation: https://vitest.dev/config/#reporters
Improve the CI configuration by updating the Codecov action and adding test results upload. Enhance test coverage reporting by including JSON format and adjusting the Vitest configuration for better output. Update the SQL formatting logic to ensure proper alignment of column definitions.