Skip to content

CLI-173 E2E test for pre-commit & pre-push hooks#119

Open
sophio-japharidze-sonarsource wants to merge 11 commits intomasterfrom
CLI-9_followup
Open

CLI-173 E2E test for pre-commit & pre-push hooks#119
sophio-japharidze-sonarsource wants to merge 11 commits intomasterfrom
CLI-9_followup

Conversation

@sophio-japharidze-sonarsource
Copy link
Contributor

No description provided.

@sonar-review-alpha
Copy link

sonar-review-alpha bot commented Mar 19, 2026

Summary

Adds comprehensive E2E tests for git hook integration (pre-commit and pre-push) supporting native hooks, Husky, and pre-commit framework. Tests verify hook installation, secret detection blocking, and error handling. Also includes guidance in CLI output on how to skip hooks using the --no-verify flag. Includes extensive supporting infrastructure: test harness, unit tests, GitHub workflows, and build scripts.

What reviewers should know

Start with tests/integration/specs/integrate/git.test.ts to understand the scope of E2E test coverage across three hook implementations. Review src/cli/commands/integrate/git/index.ts to see where user guidance on --no-verify is provided. The test setup functions (setupSonarBinDir, setupGitUser, addBareRemote) handle the complexity of setting up real git repos with hooks for testing. Key insight: tests use Bun.spawnSync to execute actual git commands with hooks, verifying real hook behavior rather than mocking. Note that this is a shallow clone with only one commit, so review the full diff to understand all changes.


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

@hashicorp-vault-sonar-prod
Copy link

hashicorp-vault-sonar-prod bot commented Mar 19, 2026

CLI-173

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: The E2E tests are well-structured and the git-shell-fragments.ts refactoring is clean, but the unit test fix has a real defect that makes it a no-op.

🗣️ Give feedback

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

🗣️ Give feedback

import { BINARY_PATH } from '../../harness/cli-runner.js';

const PATH_DELIM = process.platform === 'win32' ? ';' : ':';
function pathWithoutNodeModules(envPath: string | undefined): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is again needed to avoid calling sonar-scanner JS dependency

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

🗣️ Give feedback

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

🗣️ Give feedback

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

🗣️ Give feedback

@sophio-japharidze-sonarsource sophio-japharidze-sonarsource force-pushed the CLI-9_integrate_pre-commit_hook branch 3 times, most recently from 1281605 to eac5d17 Compare March 19, 2026 14:53
Base automatically changed from CLI-9_integrate_pre-commit_hook to master March 19, 2026 15:16
Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: Solid E2E test suite for a well-structured new command. One logic duplication worth cleaning up but nothing blocking merge.

🗣️ Give feedback

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: The refactoring of git-shell-fragments.ts is solid — extracting preCommitBody/prePushBody removes real duplication between the native and Husky variants, and replacing the hardcoded empty-tree SHA with $(printf '' | git mktree) correctly handles SHA-256 repos. The new E2E tests are well-structured: they exercise the full hook lifecycle including actual git commit/git push invocations against a bare remote. The one issue worth fixing before merge is a corrupted lock file entry.

🗣️ Give feedback

@sonarqubecloud
Copy link

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: The refactoring of shell fragments and the new E2E hook execution tests are well-structured. One test reliability bug needs fixing before merge.

🗣️ Give feedback

@@ -520,14 +533,36 @@ describe('integrateGit', () => {
performSecretInstallSpy.mockRestore();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two tests use try/catch without expect.assertions(n). If integrateGit ever stops throwing (e.g. after a refactor), the catch block is skipped and the tests pass with zero assertions — giving false confidence.

Fix with Bun's expect DSL:

await expect(
  integrateGit({ nonInteractive: true, hook: 'typo' } as unknown as IntegrateGitOptions)
).rejects.toBeInstanceOf(InvalidOptionError);

or add expect.assertions(2) at the top of each test body. Same pattern applies to the 'global install' variant at line 541.

  • Mark as noise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant