CI tests for 32bit correction + version of Go update #526
Conversation
|
🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer. |
1 similar comment
|
🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer. |
There was a problem hiding this comment.
Some small changes. And here is how you can have different golang versions in Alpine:
https://github.com/ineiti/test_ubuntu_32bits/blob/main/.github/workflows/test.yaml
TLDR: download the go-binary from the official website and add it to the PATH.
|
What is your reasoning behind testing go 1.18 and 1.22? In some past projects we had a policy of "the last two stable versions". You might fail on go 1.19, 1.20, or 1.21 with your policy. |
There was a problem hiding this comment.
- The matrix in
test_on_prstill has go 1.18 in it - why is there a
test_on_prand atest_on_pushwith similar tests in it? This leads to errors, like you can see yourself: one has the correct golang-version matrix, the other doesn't. One still has the debug output in theSetup Alpine Linux, the other doesn't.
I propose to remove thetest_on_push- if I read correctly, only theSend coverageis unique to it. Either include it in thetest_on_pr, or add anifto it.
.github/workflows/test_on_pr.yml
Outdated
| run: make test | ||
| if: ${{ matrix.size == '32b' }} | ||
| run: | | ||
| GOVERSION=${{ matrix.golang == '1.21' && '1.21.10' || '1.22.3' }} |
There was a problem hiding this comment.
Please put 1.21.10 and 1.22.3 in the matrix, so you don't have to test here. This will most probably be overlooked the day somebody will adjust the versions in the matrix above, and then things will start to fail.
.github/workflows/tests.yml
Outdated
| concurrency: | ||
| group: ci-${{ github.ref }}-test | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
Why remove this early cancellation of already running tests when a new commit is pushed in quick succession on a PR?
There was a problem hiding this comment.
Nice spot, it was an unwanted removal indeed
There was a problem hiding this comment.
We're entering Nirvana territory :)
|
It addresses @ineiti's comments of using a matrix to simplify the ci.
Resolves #522