test(upload): cover GitHub import validation (#120)#130
Conversation
parthrohit22
left a comment
There was a problem hiding this comment.
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:
-
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. -
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 thenot.toHaveBeenCalled()assertions fail. Stub each spy with.mockResolvedValue(null)(or.mockRejectedValue). -
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:
importFromGithubresolvingnull,fetchRepositoryresolvingnull, and a backend rejection — each assertingerroris set viagetErrorMessage,analyseGithubreturnsnull, andloadingreturns tofalse. These belong in this PR, not a follow-up. -
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 latestupstream/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.
b70a875 to
c036819
Compare
parthrohit22
left a comment
There was a problem hiding this comment.
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.
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
backendServicelifecycle method in every test, so a guard regression cannot make a real network request during a unit test..git, including exact-once import -> analysis -> refresh ordering, store update, selection, and final hook state.getErrorMessage503 mapping.No production code, URL policy, API contract, or backend behaviour changed.
Acceptance criteria completed
backendService.importFromGithubis called: empty/whitespace input, malformed or non-GitHub URLs, and case-insensitive duplicates all assert exact errors and no lifecycle backend calls..gitproceed through the mocked import path, preserve the expected repository name, and verify import -> analysis -> refresh ordering.Testing performed
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
devupstream/devupstream/devCloses) is used only because the issue is fully resolved