refactor: remove any and as throughout codebase#1323
refactor: remove any and as throughout codebase#1323jescalada wants to merge 45 commits intofinos:mainfrom
any and as throughout codebase#1323Conversation
…ttestation definition
…elds, remove any in src/proxy/routes/index
Sets stub types to (`ReturnType<typeof vi.fn>`) to remove the any type
…for req and stubs
…/jescalada/git-proxy into 1174-remove-any-and-as-ts-wrapup
…y, jwtAuthHandler)
… for edge case tests
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
==========================================
- Coverage 81.37% 81.33% -0.05%
==========================================
Files 67 68 +1
Lines 4735 4837 +102
Branches 819 819
==========================================
+ Hits 3853 3934 +81
- Misses 867 888 +21
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@kriswest This one's ready for review! |
|
@jescalada this has picked up some conflicts - are you able to resolve and then hit me over the head to review it? |
|
@kriswest Should be ready to review now 😃 |
|
By the way, I'm not sure which missing coverage lines aren't already dealt with in #1356, so I suggest ignoring the lost coverage here and dealing with the aftermath in that PR instead. |
kriswest
left a comment
kriswest
left a comment
There was a problem hiding this comment.
Another monster contribution, much appreciated. I've left a handful of comments for you to look at (I think a couple of removed test cases are still needed and a util function could go in for handling error message extraction - which could allow us to turn on stack traces) - however the majority LGTM.
| step.error = true; | ||
| step.log('Push failed, pre-receive hook returned an error.'); | ||
| step.setError(`Hook execution error: ${stderrTrimmed || error.message}`); | ||
| step.setError(`Hook execution error: ${stderrTrimmed || msg}`); |
There was a problem hiding this comment.
Perhaps we should always print msg here (as it was thrown) and then append stderrTrimmed if its not falsey
| const authHeader = req.headers?.authorization; | ||
|
|
||
| if (!authHeader) { | ||
| throw new Error('Authorization header is required'); |
There was a problem hiding this comment.
It is currently, although one day we could apply pullRemote to a pull chain to govern ingress, at which point this could be anonymous... but then I supposed we will want to log who the user pulling was...
Could expand the error message in case it is sent back to the client... although I wonder if we'll ever hit it - you do get prompted for auth so there is probably a request before the push where that happens.
|
|
||
| vi.mock('../../src/config', async (importOriginal) => { | ||
| const actual: any = await importOriginal(); | ||
| const actual = await importOriginal<typeof import('../../src/config')>(); |
There was a problem hiding this comment.
feels odd, but I see others doing the same...
| expect(isValidGitUrl(null as any)).toBe(false); | ||
| expect(isValidGitUrl(undefined as any)).toBe(false); | ||
| expect(isValidGitUrl(123 as any)).toBe(false); |
There was a problem hiding this comment.
These cases are still valid to test, even though Typescript is no doubt complaining about them. How about changing to:
expect(isValidGitUrl(null as unknown as string)).toBe(false);
| expect(isValidPath(null as any)).toBe(false); | ||
| expect(isValidPath(undefined as any)).toBe(false); | ||
|
|
||
| // Additional edge cases | ||
| expect(isValidPath({} as any)).toBe(false); | ||
| expect(isValidPath([] as any)).toBe(false); | ||
| expect(isValidPath(123 as any)).toBe(false); | ||
| expect(isValidPath(true as any)).toBe(false); |
| expect(isValidBranchName(null as any)).toBe(false); | ||
| expect(isValidBranchName(undefined as any)).toBe(false); |
There was a problem hiding this comment.
These should probably also be retained as test cases.
It occurs to me that if coerced to a string they may become valid, but if they were passing before they should continue to do so if you use as unknown as string
Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com>
2 participants
Fixes #1174.