OLS-1946: Fix contributing.md for uv #269
Conversation
WalkthroughThe development documentation and Makefile were updated to reflect new commands for setting up the environment and running tests. The documentation now uses updated Changes
Estimated code review effort1 (~2 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Makefile (1)
19-23: Minor typo in help string
## Run integration tests tests→## Run the integration testsPurely cosmetic but keeps
make helpoutput tidy.CONTRIBUTING.md (1)
65-69: Mention e2e tests or drop the step for consistencyThe doc calls out
make test-unitandmake test-integrationbut omits thetest-e2etarget that exists in the Makefile. Either add the third step here or explain why it is optional to avoid confusion.
| uv sync --group dev | ||
|
|
||
| # Now you can run test commands trough make targets, or prefix the rest of commands with `uv run`, eg. `uv run make test` | ||
| # activate venv | ||
| uv venv | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify virtual-env activation
uv venv prints the venv path but does not activate it.
Newcomers may try to run make … in the host interpreter and hit missing-package errors.
Consider amending:
-# activate venv
-uv venv
+# activate venv
+source "$(uv venv)/bin/activate"(or instruct to prefix every command with uv run).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uv sync --group dev | |
| # Now you can run test commands trough make targets, or prefix the rest of commands with `uv run`, eg. `uv run make test` | |
| # activate venv | |
| uv venv | |
| uv sync --group dev | |
| # activate venv | |
| source "$(uv venv)/bin/activate" |
🤖 Prompt for AI Agents
In CONTRIBUTING.md around lines 59 to 63, clarify that the command `uv venv`
only prints the virtual environment path and does not activate it. Update the
instructions to explicitly show how to activate the virtual environment in the
shell (e.g., source the activate script) or advise users to prefix commands with
`uv run` to ensure they run inside the virtual environment. This will prevent
confusion and missing-package errors for newcomers.
| test-unit: ## Run the unit tests | ||
| @echo "Running unit tests..." | ||
| @echo "Reports will be written to ${ARTIFACT_DIR}" | ||
| COVERAGE_FILE="${ARTIFACT_DIR}/.coverage.unit" uv run pytest tests/unit --cov=src --cov-report term-missing --cov-report "json:${ARTIFACT_DIR}/coverage_unit.json" --junit-xml="${ARTIFACT_DIR}/junit_unit.xml" --cov-fail-under=60 | ||
| COVERAGE_FILE="${ARTIFACT_DIR}/.coverage.unit" uv run python -m pytest tests/unit --cov=src --cov-report term-missing --cov-report "json:${ARTIFACT_DIR}/coverage_unit.json" --junit-xml="${ARTIFACT_DIR}/junit_unit.xml" --cov-fail-under=60 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Create ${ARTIFACT_DIR} before writing reports
pytest and the coverage/junit reporters assume the target directory already exists.
If ${ARTIFACT_DIR} is missing, the run silently falls back to the project root and pollutes it with artefacts.
test-unit: ## Run the unit tests
@echo "Running unit tests..."
@echo "Reports will be written to ${ARTIFACT_DIR}"
+ @mkdir -p "${ARTIFACT_DIR}"
COVERAGE_FILE="${ARTIFACT_DIR}/.coverage.unit" uv run python -m pytest tests/unit --cov=src --cov-report term-missing --cov-report "json:${ARTIFACT_DIR}/coverage_unit.json" --junit-xml="${ARTIFACT_DIR}/junit_unit.xml" --cov-fail-under=60Apply the same pre-creation in the test-integration target.
🤖 Prompt for AI Agents
In the Makefile around lines 14 to 18, the test-unit target writes coverage and
junit reports to ${ARTIFACT_DIR} without ensuring the directory exists, which
can cause files to be created in the project root if the directory is missing.
Fix this by adding a command to create the ${ARTIFACT_DIR} directory (e.g.,
mkdir -p ${ARTIFACT_DIR}) before running pytest in the test-unit target. Also,
apply the same directory creation step in the test-integration target to prevent
similar issues there.
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Thanks, tested it locally and works fine
|
|
||
| # run integration tests | ||
| make integration-tests | ||
| make test-integration |
3 participants
Description
Some fixes to make uv work.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Documentation
Chores