Skip to content

Commit c55b550

Browse files
kevinjqliuamitgilad3
kevinjqliu
authored and
amitgilad3
committed
Refactor Makefile and run s3/adls/gcs tests (apache#2125)
<!-- 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#2124 # Rationale for this change Refactor `Makefile` * Grouped commands and added comments * Added `COVERAGE` param to run test with coverage * Added `COVERAGE_FAIL_UNDER` param to specify coverage threshold to pass * Change test coverage threshold to `85` for unit tests, we are currently at `87` * Change test coverage threshold to `75` for integration tests, we are currently at `77` CI * Add s3/adls/gcs integration tests to run in CI * Run tests with coverage report Note the `gcsfs` issue was resolved by apache#2127 # Are these changes tested? Yes # Are there any user-facing changes? No <!-- In the case of user-facing changes, please add the changelog label. -->
1 parent 907876a commit c55b550

File tree

2 files changed

+118
-57
lines changed

2 files changed

+118
-57
lines changed

‎.github/workflows/python-ci.yml

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,12 @@ jobs:
6262
run: sudo apt-get update && sudo apt-get install -y libkrb5-dev # for kerberos
6363
- name: Install
6464
run: make install-dependencies
65-
- name: Linters
65+
- name: Run linters
6666
run: make lint
67-
- name: Tests
68-
run: make test-coverage-unit
67+
- name: Run unit tests with coverage
68+
run: COVERAGE=1 make test
69+
- name: Generate coverage report (85%) # Coverage threshold should only increase over time — never decrease it!
70+
run: COVERAGE_FAIL_UNDER=85 make coverage-report
6971

7072
integration-test:
7173
runs-on: ubuntu-22.04
@@ -79,8 +81,30 @@ jobs:
7981
run: sudo apt-get update && sudo apt-get install -y libkrb5-dev # for kerberos
8082
- name: Install
8183
run: make install
82-
- name: Run integration tests
83-
run: make test-coverage-integration
84+
85+
- name: Run integration tests with coverage
86+
run: COVERAGE=1 make test-integration
87+
- name: Show debug logs
88+
if: ${{ failure() }}
89+
run: docker compose -f dev/docker-compose.yml logs
90+
91+
- name: Run s3 integration tests with coverage
92+
run: COVERAGE=1 make test-s3
8493
- name: Show debug logs
8594
if: ${{ failure() }}
8695
run: docker compose -f dev/docker-compose.yml logs
96+
97+
- name: Run adls integration tests with coverage
98+
run: COVERAGE=1 make test-adls
99+
- name: Show debug logs
100+
if: ${{ failure() }}
101+
run: docker compose -f dev/docker-compose-azurite.yml logs
102+
103+
- name: Run gcs integration tests with coverage
104+
run: COVERAGE=1 make test-gcs
105+
- name: Show debug logs
106+
if: ${{ failure() }}
107+
run: docker compose -f dev/docker-compose-gcs-server.yml logs
108+
109+
- name: Generate coverage report (75%) # Coverage threshold should only increase over time — never decrease it!
110+
run: COVERAGE_FAIL_UNDER=75 make coverage-report

‎Makefile

Lines changed: 89 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -14,106 +14,143 @@
1414
# KIND, either express or implied. See the License for the
1515
# specific language governing permissions and limitations
1616
# under the License.
17+
# ========================
18+
# Configuration Variables
19+
# ========================
1720

21+
PYTEST_ARGS ?= -v # Override with e.g. PYTEST_ARGS="-vv --tb=short"
22+
COVERAGE ?= 0 # Set COVERAGE=1 to enable coverage: make test COVERAGE=1
23+
COVERAGE_FAIL_UNDER ?= 85 # Minimum coverage % to pass: make coverage-report COVERAGE_FAIL_UNDER=70
1824

19-
help: ## Display this help
20-
@awk 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m\033[0m\n"} /^[a-zA-Z_-]+:.*?##/ { printf " \033[36m%-20s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST)
25+
ifeq ($(COVERAGE),1)
26+
TEST_RUNNER = poetry run coverage run --parallel-mode --source=pyiceberg -m
27+
else
28+
TEST_RUNNER = poetry run
29+
endif
2130

2231
POETRY_VERSION = 2.1.1
23-
install-poetry: ## Ensure Poetry is installed and the correct version is being used.
32+
33+
# ============
34+
# Help Section
35+
# ============
36+
37+
##@ General
38+
39+
help: ## Display this help message
40+
@awk 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z_-]+:.*?##/ { printf " \033[36m%-25s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST)
41+
42+
# ==================
43+
# Installation Tasks
44+
# ==================
45+
46+
##@ Setup
47+
48+
install-poetry: ## Ensure Poetry is installed at the specified version
2449
@if ! command -v poetry &> /dev/null; then \
25-
echo "Poetry could not be found. Installing..."; \
50+
echo "Poetry not found. Installing..."; \
2651
pip install --user poetry==$(POETRY_VERSION); \
2752
else \
2853
INSTALLED_VERSION=$$(pip show poetry | grep Version | awk '{print $$2}'); \
2954
if [ "$$INSTALLED_VERSION" != "$(POETRY_VERSION)" ]; then \
30-
echo "Poetry version $$INSTALLED_VERSION does not match required version $(POETRY_VERSION). Updating..."; \
55+
echo "Updating Poetry to version $(POETRY_VERSION)..."; \
3156
pip install --user --upgrade poetry==$(POETRY_VERSION); \
3257
else \
33-
echo "Poetry version $$INSTALLED_VERSION is already installed."; \
34-
fi \
58+
echo "Poetry version $(POETRY_VERSION) already installed."; \
59+
fi; \
3560
fi
3661

37-
install-dependencies: ## Install dependencies including dev, docs, and all extras
62+
install-dependencies: ## Install all dependencies including extras
3863
poetry install --all-extras
3964

40-
install: | install-poetry install-dependencies
65+
install: install-poetry install-dependencies ## Install Poetry and dependencies
66+
67+
# ===============
68+
# Code Validation
69+
# ===============
70+
71+
##@ Quality
4172

4273
check-license: ## Check license headers
4374
./dev/check-license
4475

45-
lint: ## lint
76+
lint: ## Run code linters via pre-commit
4677
poetry run pre-commit run --all-files
4778

48-
test: ## Run all unit tests, can add arguments with PYTEST_ARGS="-vv"
49-
poetry run pytest tests/ -m "(unmarked or parametrize) and not integration" ${PYTEST_ARGS}
79+
# ===============
80+
# Testing Section
81+
# ===============
5082

51-
test-s3: # Run tests marked with s3, can add arguments with PYTEST_ARGS="-vv"
52-
sh ./dev/run-minio.sh
53-
poetry run pytest tests/ -m s3 ${PYTEST_ARGS}
83+
##@ Testing
5484

55-
test-integration: | test-integration-setup test-integration-exec ## Run all integration tests, can add arguments with PYTEST_ARGS="-vv"
85+
test: ## Run all unit tests (excluding integration)
86+
$(TEST_RUNNER) pytest tests/ -m "(unmarked or parametrize) and not integration" $(PYTEST_ARGS)
5687

57-
test-integration-setup: # Prepare the environment for integration
88+
test-integration: test-integration-setup test-integration-exec ## Run integration tests
89+
90+
test-integration-setup: ## Start Docker services for integration tests
5891
docker compose -f dev/docker-compose-integration.yml kill
5992
docker compose -f dev/docker-compose-integration.yml rm -f
6093
docker compose -f dev/docker-compose-integration.yml up -d
6194
sleep 10
6295
docker compose -f dev/docker-compose-integration.yml cp ./dev/provision.py spark-iceberg:/opt/spark/provision.py
6396
docker compose -f dev/docker-compose-integration.yml exec -T spark-iceberg ipython ./provision.py
6497

65-
test-integration-exec: # Execute integration tests, can add arguments with PYTEST_ARGS="-vv"
66-
poetry run pytest tests/ -v -m integration ${PYTEST_ARGS}
98+
test-integration-exec: ## Run integration tests (excluding provision)
99+
$(TEST_RUNNER) pytest tests/ -m integration $(PYTEST_ARGS)
67100

68-
test-integration-rebuild:
101+
test-integration-rebuild: ## Rebuild integration Docker services from scratch
69102
docker compose -f dev/docker-compose-integration.yml kill
70103
docker compose -f dev/docker-compose-integration.yml rm -f
71104
docker compose -f dev/docker-compose-integration.yml build --no-cache
72105

73-
test-adls: ## Run tests marked with adls, can add arguments with PYTEST_ARGS="-vv"
106+
test-s3: ## Run tests marked with @pytest.mark.s3
107+
sh ./dev/run-minio.sh
108+
$(TEST_RUNNER) pytest tests/ -m s3 $(PYTEST_ARGS)
109+
110+
test-adls: ## Run tests marked with @pytest.mark.adls
74111
sh ./dev/run-azurite.sh
75-
poetry run pytest tests/ -m adls ${PYTEST_ARGS}
112+
$(TEST_RUNNER) pytest tests/ -m adls $(PYTEST_ARGS)
76113

77-
test-gcs: ## Run tests marked with gcs, can add arguments with PYTEST_ARGS="-vv"
114+
test-gcs: ## Run tests marked with @pytest.mark.gcs
78115
sh ./dev/run-gcs-server.sh
79-
poetry run pytest tests/ -m gcs ${PYTEST_ARGS}
116+
$(TEST_RUNNER) pytest tests/ -m gcs $(PYTEST_ARGS)
80117

81-
test-coverage-unit: # Run test with coverage for unit tests, can add arguments with PYTEST_ARGS="-vv"
82-
poetry run coverage run --source=pyiceberg/ --data-file=.coverage.unit -m pytest tests/ -v -m "(unmarked or parametrize) and not integration" ${PYTEST_ARGS}
83-
84-
test-coverage-integration: # Run test with coverage for integration tests, can add arguments with PYTEST_ARGS="-vv"
85-
docker compose -f dev/docker-compose-integration.yml kill
86-
docker compose -f dev/docker-compose-integration.yml rm -f
87-
docker compose -f dev/docker-compose-integration.yml up -d
88-
sh ./dev/run-azurite.sh
89-
sh ./dev/run-gcs-server.sh
90-
sleep 10
91-
docker compose -f dev/docker-compose-integration.yml cp ./dev/provision.py spark-iceberg:/opt/spark/provision.py
92-
docker compose -f dev/docker-compose-integration.yml exec -T spark-iceberg ipython ./provision.py
93-
poetry run coverage run --source=pyiceberg/ --data-file=.coverage.integration -m pytest tests/ -v -m integration ${PYTEST_ARGS}
118+
test-coverage: COVERAGE=1
119+
test-coverage: test test-integration test-s3 test-adls test-gcs coverage-report ## Run all tests with coverage and report
94120

95-
test-coverage: | test-coverage-unit test-coverage-integration ## Run all tests with coverage including unit and integration tests
96-
poetry run coverage combine .coverage.unit .coverage.integration
97-
poetry run coverage report -m --fail-under=90
121+
coverage-report: ## Combine and report coverage
122+
poetry run coverage combine
123+
poetry run coverage report -m --fail-under=$(COVERAGE_FAIL_UNDER)
98124
poetry run coverage html
99125
poetry run coverage xml
100126

127+
# ================
128+
# Documentation
129+
# ================
101130

102-
clean: ## Clean up the project Python working environment
103-
@echo "Cleaning up Cython and Python cached files"
104-
@rm -rf build dist *.egg-info
105-
@find . -name "*.so" -exec echo Deleting {} \; -delete
106-
@find . -name "*.pyc" -exec echo Deleting {} \; -delete
107-
@find . -name "__pycache__" -exec echo Deleting {} \; -exec rm -rf {} +
108-
@find . -name "*.pyd" -exec echo Deleting {} \; -delete
109-
@find . -name "*.pyo" -exec echo Deleting {} \; -delete
110-
@echo "Cleanup complete"
131+
##@ Documentation
111132

112-
docs-install:
133+
docs-install: ## Install docs dependencies
113134
poetry install --with docs
114135

115-
docs-serve:
136+
docs-serve: ## Serve local docs preview (hot reload)
116137
poetry run mkdocs serve -f mkdocs/mkdocs.yml
117138

118-
docs-build:
139+
docs-build: ## Build the static documentation site
119140
poetry run mkdocs build -f mkdocs/mkdocs.yml --strict
141+
142+
# ===================
143+
# Project Maintenance
144+
# ===================
145+
146+
##@ Maintenance
147+
148+
clean: ## Remove build artifacts and caches
149+
@echo "Cleaning up Cython and Python cached files..."
150+
@rm -rf build dist *.egg-info
151+
@find . -name "*.so" -exec echo Deleting {} \; -delete
152+
@find . -name "*.pyc" -exec echo Deleting {} \; -delete
153+
@find . -name "__pycache__" -exec echo Deleting {} \; -exec rm -rf {} +
154+
@find . -name "*.pyd" -exec echo Deleting {} \; -delete
155+
@find . -name "*.pyo" -exec echo Deleting {} \; -delete
156+
@echo "Cleanup complete."

0 commit comments

Comments
 (0)