Merge python-integration.yml into python-ci.yml #1963
Conversation
.github/workflows/python-ci.yml
Outdated
| - name: Install | ||
| run: make install | ||
| - name: Run integration tests | ||
| run: make test-integration |
There was a problem hiding this comment.
I dont think we want to do this.
L68's test-coverage runs both the unit tests and integrations tests
This line here runs the integration tests again.
I think we'd want to get rid of one OR have one run unit tests and another run integration tests.
Looking at both github actions. python-ci runs the matrix for python v3.9-3.12 whereas python-integration only runs for the default python version.
There was a problem hiding this comment.
Ah, nice catch. Personally, I like the idea of having them stay separate, so they can run in parallel. I pushed a change separating them while still running the full Python version matrix for both unit and integration tests with coverage. What do you think?
| - name: Linters | ||
| run: make lint | ||
| - name: Tests | ||
| run: make test-coverage |
There was a problem hiding this comment.
Ah, I see. This will also run the integration tests 🤣
.github/workflows/python-ci.yml
Outdated
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 2 |
There was a problem hiding this comment.
Any specific reason for depth 2? The default is 1.
There was a problem hiding this comment.
This was copy and paste from the old python-integration.yml file. I did however check on who introduced it in the first place... and it turns out it was you at this commit :)
Shall I remove it? I don't see a reason either?
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! just small change on the fetch-depth
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
Closes apache#1942
# Rationale for this change
This condenses similar CI logic into one place
# Are these changes tested?
# Are there any user-facing changes?
No
<!-- In the case of user-facing changes, please add the changelog label.
-->
3 participants
Closes #1942
Rationale for this change
This condenses similar CI logic into one place
Are these changes tested?
Are there any user-facing changes?
No