Update dev docs with recent information on how to run tests#240
Merged
Update dev docs with recent information on how to run tests#240
Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates contributing and installation documentation to match changes in the Docker Compose-based development setup: replaces Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- There are a couple of small wording/typo issues in the new docs (e.g.
There three important...should beThere are three important...,implemting->implementing, andElastic searchis usually writtenElasticsearch) that would be good to clean up for clarity. - The description for the
php_apimarker currently ends withThese are tests whichand seems incomplete; consider finishing that sentence so readers understand exactly what distinguishes those tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are a couple of small wording/typo issues in the new docs (e.g. `There three important...` should be `There are three important...`, `implemting` -> `implementing`, and `Elastic search` is usually written `Elasticsearch`) that would be good to clean up for clarity.
- The description for the `php_api` marker currently ends with `These are tests which` and seems incomplete; consider finishing that sentence so readers understand exactly what distinguishes those tests.
## Individual Comments
### Comment 1
<location> `docs/contributing/contributing.md:48` </location>
<code_context>
For example, visit [http://localhost:8002/api/v1/json/data/1](http://localhost:8002/api/v1/json/data/1)
to fetch a JSON description of dataset 1.
- - `python-api`: this container serves the new Python-based REST API at `localhost:8001`.
+ - `openml-elasticsearch`: Elastic search, required for the PHP REST API to function.
+ - `openml-python-rest-api`: this container serves the new Python-based REST API at `localhost:8001`.
For example, visit [http://localhost:8001/docs](http://localhost:8001/docs) to see
</code_context>
<issue_to_address>
**suggestion (typo):** Use the correct product name "Elasticsearch" instead of "Elastic search".
For consistency with the container name (`openml-elasticsearch`) and the official product name, please change "Elastic search" to "Elasticsearch".
Suggested implementation:
```
- `openml-elasticsearch`: Elasticsearch, required for the PHP REST API to function.
```
```
On arm-based Macs, you need to enable Rosetta emulation for Docker for the Elasticsearch container to work.
```
</issue_to_address>
### Comment 2
<location> `docs/contributing/contributing.md:54-55` </location>
<code_context>
the REST API documentation. Changes to the code in `src/` will be reflected in this
container.
+!!! note
+ On arm-based Macs, you need to enable Rosetta emulation for Docker for the Elastic Search container to work.
+
+We can now run the full test suite, which takes about 4 minutes:
</code_context>
<issue_to_address>
**suggestion (typo):** Consistently spell the product name as "Elasticsearch".
Here it’s written as "Elastic Search"; please change this to "Elasticsearch" to match the official name.
Suggested implementation:
```
- `openml-elasticsearch`: Elasticsearch, required for the PHP REST API to function.
```
```
!!! note
On arm-based Macs, you need to enable Rosetta emulation for Docker for the Elasticsearch container to work.
```
</issue_to_address>
### Comment 3
<location> `docs/contributing/contributing.md:62` </location>
<code_context>
+```bash
+docker exec openml-python-rest-api python -m pytest tests
+```
+There three important [test markers](https://docs.pytest.org/en/7.1.x/example/markers.html) to be aware of:
+
+ - `php_api`: all tests that require the PHP API container. These are tests which
</code_context>
<issue_to_address>
**issue (typo):** Add the missing verb in "There three important...".
This reads as if a word is missing; please change to "There are three important [test markers]...".
```suggestion
There are three important [test markers](https://docs.pytest.org/en/7.1.x/example/markers.html) to be aware of:
```
</issue_to_address>
### Comment 4
<location> `docs/contributing/contributing.md:68` </location>
<code_context>
+ - `python_api`: all tests that require the Python API container. That's almost all of them.
+ - `slow`: for long-running tests. Currently only one test.
+
+In many cases during development it's sufficient to either run with `not php_api and not slow` when initially adding the endpoint and implemting its response, or later `php_api and not slow` when working on the 'migration' tests that validate against the old PHP API.
+The `not slow` is only needed if a slow test would be included in your test selection. In many cases, you might prefer to only run the specific tests (or test modules) that you are working on and excluding it through markers may be unnecessary.
+Examples:
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo "implemting" to "implementing".
Typo: change "implemting" to "implementing".
```suggestion
In many cases during development it's sufficient to either run with `not php_api and not slow` when initially adding the endpoint and implementing its response, or later `php_api and not slow` when working on the 'migration' tests that validate against the old PHP API.
```
</issue_to_address>
### Comment 5
<location> `docs/contributing/contributing.md:119` </location>
<code_context>
+python -m pytest -v -x --lf -m "not php_api"
```
Where `-v` show the name of each test ran, `-x` ensures testing stops on first failure,
-`--lf` will first run the test(s) which failed last, and `-m "not web"` specifies
-which tests (not) to run.
</code_context>
<issue_to_address>
**suggestion (typo):** Fix subject-verb agreement and past participle in "show" and "ran".
Consider: "Where `-v` shows the name of each test run, `-x` ensures testing stops on the first failure."
```suggestion
Where `-v` shows the name of each test run, `-x` ensures testing stops on the first failure,
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/contributing/contributing.md (1)
28-29:⚠️ Potential issue | 🟡 MinorFix typo: “projected” → “project” (or “project is”).
This reads incorrectly and is confusing in setup instructions.
✅ Suggested edit
-With the projected forked, cloned, and installed, the easiest way to set up all +With the project forked, cloned, and installed, the easiest way to set up all
🤖 Fix all issues with AI agents
In `@docs/contributing/contributing.md`:
- Line 73: Remove the trailing space inside the inline code span containing the
pytest command so the fenced text reads `docker exec openml-python-rest-api
python -m pytest tests/routers/openml/dataset_tag_test.py` (no trailing space);
edit the contributing.md line that currently has `dataset_tag_test.py ` to
remove the extra space character within the backticks to satisfy markdownlint
MD038.
- Around line 62-68: Fix typos and tighten the test marker paragraph: change
“There three important” to “There are three important” (or “Three important”),
correct “implemting” to “implementing”, and rephrase the final sentence for
clarity (e.g. recommend running tests with `not php_api and not slow` during
initial endpoint work, and `php_api and not slow` when working on migration
tests). Update the three marker bullets (`php_api`, `python_api`, `slow`) if
needed to keep wording concise and consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.