ci: add Windows to test matrix and fix cross-platform compatibility #1361
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1361 +/- ##
=======================================
Coverage 80.40% 80.40%
=======================================
Files 65 65
Lines 4608 4608
Branches 775 775
=======================================
Hits 3705 3705
Misses 888 888
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fabiovincenzi
requested review from
a team,
jescalada and
kriswest
and removed request for
kriswest
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
The tests look good!
Wondering if we could keep the MongoDB combinations in the CI and think about whether we need a Windows CI execution since Windows test support is more of a quality-of-life thing for Windows development...
| on: | ||
| push: | ||
| branches: [main] | ||
| branches: [main, windows-failing-tests] |
There was a problem hiding this comment.
We might want to revert this if not testing anymore!
| branches: [main, windows-failing-tests] | |
| branches: [main] |
There was a problem hiding this comment.
You are right used for testing
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
|
|
||
| - name: Start MongoDB |
There was a problem hiding this comment.
Any specific reason to remove MongoDB from the test matrix and steps?
There was a problem hiding this comment.
I noticed that we already have 6 CI combinations (20, 22, 24) x (Windows, Ubuntu), so adding the 3 Mongo versions would result in 18 CI runs, spending way too many resources in CI runs.
Perhaps we could keep the CI execution as it is, and then add a ci-windows.yml file separately? Not sure if this makes the most sense - Windows is not really meant to be a "supported" OS beyond development since GitProxy would usually be deployed on a Unix machine.
There was a problem hiding this comment.
MongoDB was originally added to the CI in November 2020 (f8d5f9f) when it was the default database.
However, at some point the default was changed to use the file-based database, and now MongoDB is enabled: false by default in proxy.config.json. Additionally, when NODE_ENV=test, the tests use neDB in-memory (see src/db/file/*.ts with inMemoryOnly: true), so they never actually connect to MongoDB.
This means MongoDB has been running in CI without being used by tests for quite some time.
Regarding Windows: I agree it's more of a quality-of-life feature for development. Having 6 combinations (3 Node × 2 OS) seems reasonable to me, but I'm open to moving Windows to a separate workflow if preferred. What do you think?
| vi.mock('fs', { spy: true }); | ||
|
|
||
| describe('Pre-Receive Hook Execution', () => { | ||
| describe.skipIf(process.platform === 'win32')('Pre-Receive Hook Execution', () => { |
There was a problem hiding this comment.
Might as well add a warning here and in prereceive.ts in the rare case this is being used on Windows 🤔
| describe.skipIf(process.platform === 'win32')('Pre-Receive Hook Execution', () => { | |
| // Pre-receive is not currently supported on Windows | |
| describe.skipIf(process.platform === 'win32')('Pre-Receive Hook Execution', () => { |
| 'git', | ||
| ['config', 'receive.unpackLimit', '0'], | ||
| expect.objectContaining({ cwd: '/path/to/repo' }), | ||
| expect.objectContaining({ cwd: path.join('path', 'to', 'repo') }), |
Signed-off-by: Fabio Vincenzi <[email protected]>
2 participants
Fixes: #1318.
This PR adds Windows to the CI test matrix and fixes cross-platform compatibility issues.
CI Changes
Cross-Platform Fixes
The preReceive tests are skipped because they test Unix shell hook execution, which is not applicable on Windows.