Skip to content

test(upload): cover GitHub import validation (#120)#130

Merged
parthrohit22 merged 2 commits into
Second-Origin:devfrom
hardikuppal04:test/120-github-import-validation
Jul 18, 2026
Merged

test(upload): cover GitHub import validation (#120)#130
parthrohit22 merged 2 commits into
Second-Origin:devfrom
hardikuppal04:test/120-github-import-validation

Conversation

@hardikuppal04

@hardikuppal04 hardikuppal04 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds hermetic regression coverage for the existing GitHub-import validation and lifecycle. The hook already handled these paths, but its guards and failure behaviour were not directly protected from regressions.

Linked issue

Closes #120

What changed

Frontend

  • Added focused tests for empty, whitespace-only, malformed, non-GitHub, incomplete, over-specified, and case-insensitive duplicate GitHub repository inputs.
  • Stubbed every backendService lifecycle method in every test, so a guard regression cannot make a real network request during a unit test.
  • Added success coverage for HTTPS URLs with and without .git, including exact-once import -> analysis -> refresh ordering, store update, selection, and final hook state.
  • Added failure coverage for a null import result, a null refresh result, and a rejected backend request using the real getErrorMessage 503 mapping.

No production code, URL policy, API contract, or backend behaviour changed.

Acceptance criteria completed

  • Tests cover every validation branch before backendService.importFromGithub is called: empty/whitespace input, malformed or non-GitHub URLs, and case-insensitive duplicates all assert exact errors and no lifecycle backend calls.
  • Tests cover the duplicate-name guard case-insensitively.
  • Valid HTTPS GitHub URLs with and without .git proceed through the mocked import path, preserve the expected repository name, and verify import -> analysis -> refresh ordering.
  • Tests pass in the standard frontend CI command.

Testing performed

PS apps/frontend> npm.cmd run lint
> eslint .

PS apps/frontend> npm.cmd run test
Test Files  10 passed (10)
Tests  52 passed (52)
Coverage thresholds passed

PS apps/frontend> npm.cmd run build
> tsc -b && vite build
✓ built in 2.27s

Screenshots

Not applicable — this PR adds unit tests only and does not change visible UI behaviour.

Security and data considerations

No production security or data-handling change. Every backend lifecycle method is stubbed in the hook tests, so no real network request, credential, repository data, or user data is used during the test run.

Dependencies and blocked work

None.

Scope changes or remaining work

None. The change is intentionally limited to regression coverage for the existing hook behaviour.

Contributor checklist

  • This PR targets dev
  • I claimed the issue and had it assigned or acknowledged before starting substantial work
  • The branch was created from an up-to-date upstream/dev
  • The branch is rebased on the latest upstream/dev
  • This PR addresses one clearly scoped issue
  • Every acceptance criterion I claim as complete is actually complete
  • Relevant tests pass
  • Documentation is updated for any user-visible change
  • No secrets, credentials, local env files, or generated artifacts are included
  • No unrelated files were changed
  • Closing syntax (Closes) is used only because the issue is fully resolved
  • Dependencies and follow-up work are linked

@SHAURYAKSHARMA24
SHAURYAKSHARMA24 marked this pull request as ready for review July 18, 2026 17:34
@SHAURYAKSHARMA24
SHAURYAKSHARMA24 self-requested a review July 18, 2026 19:17

@parthrohit22 parthrohit22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes. The test content itself has been independently verified — I ran the suite with this file against dev (49/49 passing), every pre-backend validation branch of the hook maps to a test with exact error-message and backend-not-called assertions, and the functional requirements of #120 are met. What's blocking is everything around the code, plus two hardening gaps:

  1. Rewrite the PR body to follow .github/pull_request_template.md. The current body skips most of the required template: ## Linked issue, ## Acceptance criteria completed (map each criterion from #120 explicitly and state whether it's complete), ## Screenshots, ## Security and data considerations, ## Dependencies and blocked work, ## Scope changes or remaining work — and the entire Contributor checklist is missing. The checklist is how this repo holds claims accountable; it is not optional, and every box you tick must be genuinely true.

  2. Make the rejection tests hermetic. The three vi.spyOn(backendService, ...) calls in the empty-URL, invalid-URL, and duplicate-name tests keep the real implementations. If validation ever regresses, those tests would fire real network requests before the not.toHaveBeenCalled() assertions fail. Stub each spy with .mockResolvedValue(null) (or .mockRejectedValue).

  3. Back the "complete import lifecycle" claim. The body claims the complete lifecycle is covered, but only the success path is tested. Add failure-path tests: importFromGithub resolving null, fetchRepository resolving null, and a backend rejection — each asserting error is set via getErrorMessage, analyseGithub returns null, and loading returns to false. These belong in this PR, not a follow-up.

  4. Rebase on the latest dev. #126, #127, and #128 have merged since this branch was cut — #127 landed after your branch point, so you're now behind base. No conflicts are expected (your change is one frontend test file; #127 is backend-only), but the contributor checklist requires the branch be rebased on latest upstream/dev, and CI needs to run against the current base. After rebasing, re-run lint, the full test suite, and the build, and paste the real output in the Testing section — the test count will change once your new failure-path tests are in, so don't reuse the old numbers.

One more process note to @SHAURYAKSHARMA24 : the existing approval on this PR has an empty body and predates any posted review feedback. Approvals here need to state what was actually verified — please don't treat that approval as merge-readiness.

Fix these and this is an easy approve — the underlying test design (call-order assertions via invocationCallOrder, real store integration, case-insensitive duplicate coverage) is genuinely good work.

@parthrohit22 parthrohit22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed after the rebase and c036819 — all four requested items are complete, and I verified each independently rather than from the PR body: ran the suite with this file against current dev (52/52 passing, matching your reported numbers exactly), and confirmed the merge-base equals the current dev head, so the rebase claim is genuine.

Highlights: the mockBackendLifecycle() helper makes every test hermetic by default, and the failure-path tests are stronger than requested — asserting exact error messages, call-count boundaries after each failure point, and the real ApiError(503) → user-facing message mapping through getErrorMessage.

The template rewrite with the honest per-criterion mapping and testing output is exactly the standard this repo needs. Approving — good turnaround.

@parthrohit22
parthrohit22 merged commit ebd36a7 into Second-Origin:dev Jul 18, 2026
7 checks passed
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.

chore: add unit coverage for GitHub import validation and duplicate detection

3 participants