CI: add Lint Check step to build in Build, Test and Publish Action#354
Open
Carbonautics wants to merge 3 commits intoopenfga:mainfrom
Open
CI: add Lint Check step to build in Build, Test and Publish Action#354Carbonautics wants to merge 3 commits intoopenfga:mainfrom
Carbonautics wants to merge 3 commits intoopenfga:mainfrom
Conversation
WalkthroughThis PR adds a lint check to the CI/CD pipeline and reorganizes test infrastructure. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
=======================================
Coverage 85.80% 85.80%
=======================================
Files 26 26
Lines 1268 1268
Branches 225 225
=======================================
Hits 1088 1088
Misses 110 110
Partials 70 70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #275 by ensuring linting is executed during CI so builds fail when ESLint reports errors, helping keep the TypeScript SDK codebase consistent and lint-clean.
Changes:
- Add a
npm run lintstep to the GitHub Actions build job beforenpm run build. - Reformat Jest test files to comply with the repository’s ESLint formatting rules.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.github/workflows/main.yaml |
Runs ESLint in CI prior to building so lint errors fail the workflow. |
tests/setup.ts |
Formatting-only change to satisfy lint rules. |
tests/apiExecutor.test.ts |
Formatting-only change (large diff) to satisfy lint rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+129
to
+131
| // If we get here without error, the Content-Type header was correctly applied | ||
| expect(true).toBe(true); | ||
| }); |
Comment on lines
36
to
+41
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Lint Check | ||
| run: npm run lint |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/main.yaml (1)
19-21: Run lint once instead of once per matrix entry to cut CI time.This currently runs the same lint pass 4x. Consider gating lint to one Node version while keeping
npm run buildon the full matrix.♻️ Suggested adjustment
- name: Lint Check + if: matrix.node-version == env.TARGET_NODE_VERSION run: npm run lintAlso applies to: 40-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/main.yaml around lines 19 - 21, The workflow currently runs the same lint step for every entry in strategy.matrix; update the YAML so lint runs just once by removing the matrix from the lint job (or adding a conditional like if: github.matrix.node-version == '22.x') and keep the build/test job under strategy.matrix for full matrix coverage; locate the strategy.matrix block and the jobs named "lint" and "build" (or equivalent job IDs) and either move lint out as a standalone job without a matrix or add an if-gate tied to a single node-version to ensure lint runs only once while builds still run across all node-version entries.tests/apiExecutor.test.ts (1)
19-32: Consider extracting duplicated Nock hook setup into a helper.Both suites define the same lifecycle hooks; a small helper would reduce drift and keep this easier to maintain.
♻️ Possible refactor
+const registerNockLifecycleHooks = () => { + beforeAll(() => { + nock.restore(); + nock.cleanAll(); + nock.activate(); + nock.disableNetConnect(); + }); + + afterEach(() => { + nock.cleanAll(); + }); + + afterAll(() => { + nock.restore(); + }); +}; + describe("OpenFgaClient.executeApiRequest", () => { + registerNockLifecycleHooks(); const basePath = defaultConfiguration.getBasePath(); ... - beforeAll(() => { ... }); - afterEach(() => { ... }); - afterAll(() => { ... }); }); describe("OpenFgaClient.executeApiRequest - path parameters", () => { + registerNockLifecycleHooks(); const basePath = defaultConfiguration.getBasePath(); ... - beforeAll(() => { ... }); - afterEach(() => { ... }); - afterAll(() => { ... }); });Also applies to: 340-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apiExecutor.test.ts` around lines 19 - 32, Duplicate Nock lifecycle hook setup in tests/apiExecutor.test.ts (the beforeAll/afterEach/afterAll blocks that call nock.restore(), nock.cleanAll(), nock.activate(), nock.disableNetConnect()) should be extracted into a shared helper; create a function (e.g., setupNockLifecycle or useNockHooks) that registers the same beforeAll, afterEach, and afterAll behavior and call that helper from each test suite instead of repeating the hooks—update references in the suites to invoke the helper and remove the duplicated hook blocks so both suites use the single shared setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/main.yaml:
- Around line 19-21: The workflow currently runs the same lint step for every
entry in strategy.matrix; update the YAML so lint runs just once by removing the
matrix from the lint job (or adding a conditional like if:
github.matrix.node-version == '22.x') and keep the build/test job under
strategy.matrix for full matrix coverage; locate the strategy.matrix block and
the jobs named "lint" and "build" (or equivalent job IDs) and either move lint
out as a standalone job without a matrix or add an if-gate tied to a single
node-version to ensure lint runs only once while builds still run across all
node-version entries.
In `@tests/apiExecutor.test.ts`:
- Around line 19-32: Duplicate Nock lifecycle hook setup in
tests/apiExecutor.test.ts (the beforeAll/afterEach/afterAll blocks that call
nock.restore(), nock.cleanAll(), nock.activate(), nock.disableNetConnect())
should be extracted into a shared helper; create a function (e.g.,
setupNockLifecycle or useNockHooks) that registers the same beforeAll,
afterEach, and afterAll behavior and call that helper from each test suite
instead of repeating the hooks—update references in the suites to invoke the
helper and remove the duplicated hook blocks so both suites use the single
shared setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5630d977-c4e8-4413-8ec5-018bc69234ea
📒 Files selected for processing (3)
.github/workflows/main.yamltests/apiExecutor.test.tstests/setup.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an ESLint linting gate to the CI build workflow so builds fail on lint errors, and applies lint/format fixes across the codebase (as reflected in updated test files).
Changes:
- Add a
npm run lintstep to the GitHub Actions build job beforenpm run build. - Apply lint-driven formatting updates in tests (indentation/style consistency).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
.github/workflows/main.yaml |
Runs npm run lint in CI prior to building to fail fast on lint errors. |
tests/setup.ts |
Formatting-only change aligning with lint rules. |
tests/apiExecutor.test.ts |
Formatting-only changes aligning with lint rules (no functional test logic changes intended). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Lint Check |
Comment on lines
+129
to
+130
| // If we get here without error, the Content-Type header was correctly applied | ||
| expect(true).toBe(true); |
Comment on lines
+150
to
+151
| // If we get here without error, the Accept header was correctly applied | ||
| expect(true).toBe(true); |
Comment on lines
+232
to
+233
| // If we get here without error, the custom header was correctly applied | ||
| expect(true).toBe(true); |
Comment on lines
+258
to
+259
| // If we get here without error, the custom header was correctly applied | ||
| expect(true).toBe(true); |
| } | ||
| ); | ||
|
|
||
| expect(true).toBe(true); |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
What problem is being solved?
How is it being solved?
npm run lintscript after installing dependencies usingnpm cibut before actually creating a build.npm run lint:fixlocally and push all fixes to any required files.What changes are made to solve it?
.github/workflows/main.yamlfile to include the lint check command.npm run lint:fix, since there were 400+ errors which would fail everytime, if we added this lint check to the build without fixing those lint/formatting errors first.References
closes #275
Review Checklist
mainSummary by CodeRabbit