Python: Make Python CI collect most of the unit tests and Fix CI name conflict #7136
Conversation
| # | ||
|
|
||
| name: "Python CI" | ||
| name: "Python Integration" |
There was a problem hiding this comment.
I observe that If two CI have the same name, the following error occurs sometimes:
Probably the reason that Python CI fails(get cancelled) sometimes....
python/Makefile
Outdated
| test: | ||
| poetry run coverage run --source=pyiceberg/ -m pytest tests/ -m unmarked ${PYTEST_ARGS} | ||
| poetry run coverage report -m --fail-under=90 | ||
| poetry run coverage report -m --fail-under=88 |
There was a problem hiding this comment.
The current test coverage excluding integration, s3, and adlfs tests is 88%
There was a problem hiding this comment.
We definitely want to keep this 90, but it's probably more important to at least run the tests and we don't want to blindly add tests just for the sake of making it 90, I will approve the PR, but can you create an Issue tracking that so we take a deeper look at where we can add more tests?
There was a problem hiding this comment.
Good point! I created a new issue: #7149.
Since the drop of test coverage is caused of exlusion of some tests related to s3 and adlfs, adding them back when computing the test coverage may also be a solution. I argee that the top priority is to make these tests running in the CI, so I will keep the threshold at 88% as a temporal solution.
There was a problem hiding this comment.
@JonasJ-ap I think this only runs the tests that are marked as unmarked and checks the code coverage. How about adding another target in the Makefile that will run all the test of all the markers (unmarked, s3, adlfs, and gcs soon), and only add the code coverage check there. Right now we expect all the test suites to cover 90%+, which is not really needed.
There was a problem hiding this comment.
@Fokko Thank you for your suggestion. I added a new target called "test-coverage" to run all the unit tests and make this the only command to be run in the "Python CI" workflow. I also noticed that some tests utilizing @pytest.mark.parametrize(...) are marked as "parametrize", so I also included these in the "test" target.
JonasJ-ap
changed the title
python/Makefile
Outdated
| test: | ||
| poetry run coverage run --source=pyiceberg/ -m pytest tests/ -m unmarked ${PYTEST_ARGS} | ||
| poetry run coverage report -m --fail-under=90 | ||
| poetry run coverage report -m --fail-under=88 |
There was a problem hiding this comment.
We definitely want to keep this 90, but it's probably more important to at least run the tests and we don't want to blindly add tests just for the sake of making it 90, I will approve the PR, but can you create an Issue tracking that so we take a deeper look at where we can add more tests?
|
@jackye1995 Thanks for your review and suggestions. @Fokko May I ask if you have time to take a look at this PR? |
JonasJ-ap
changed the title
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks @JonasJ-ap this is great, thanks for picking this up. I left a few comments, let me know what you think
python/Makefile
Outdated
| test: | ||
| poetry run coverage run --source=pyiceberg/ -m pytest tests/ -m unmarked ${PYTEST_ARGS} | ||
| poetry run coverage report -m --fail-under=90 | ||
| poetry run coverage report -m --fail-under=88 |
There was a problem hiding this comment.
@JonasJ-ap I think this only runs the tests that are marked as unmarked and checks the code coverage. How about adding another target in the Makefile that will run all the test of all the markers (unmarked, s3, adlfs, and gcs soon), and only add the code coverage check there. Right now we expect all the test suites to cover 90%+, which is not really needed.
3 participants
Closes #7135
Closes #7149