Details
- Reviewers
vlorentz - Group Reviewers
Reviewers - Commits
- rDCIDXcf5c1faf8e3a: tests: Align Language indexer tests with other content indexer tests
rDCIDX0222c9360721: tests: Update docstrings
rDCIDXa3f15fc22862: tests: Factorize common behavior between content indexers
rDCIDXdc0e879a5f86: fossology_license: Open new range indexer
tox
Diff Detail
- Repository
- rDCIDX Metadata indexer
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Build has FAILED
Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/50/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/50/console
It seems big but it's mostly because the initial tests have been extracted, refactored and moved around...
| swh/indexer/fossology_license.py | ||
|---|---|---|
| 171–172 | No need for that break | |
| swh/indexer/tests/test_fossology_license.py | ||
| 51–65 | I'd rather call super().prepare(), then override stuff. Less code duplication, and it tests the actual prepare. | |
| 100 | Having a expected_results in a setUp is weird. Maybe add an assert_indexer_result method to CommonContentIndexerTest instead? | |
| 125–143 | same as above | |
| 157–183 | same as above. | |
| swh/indexer/tests/test_language.py | ||
| 66–88 ↗ | (On Diff #2112) | same |
| swh/indexer/tests/test_mimetype.py | ||
| 140–159 | same | |
| swh/indexer/tests/test_utils.py | ||
| 584 | double underscore | |
| 595 | double underscore | |
I disagree with your systematic 'require changes' policy approach.
There is nothing wrong in that diff that would prevent a merge.
There are:
- docstrings in the right format
- docstrings for each new endpoint opened (even in tests)
- tests
- and refactorized behavior for tests (thus the length of the diff)
There is one useless conditional but that's it.
The only blocker for me here would be the build failure.
And even that is a current limitation of the jenkins tool setup which will be adressed in time.
| swh/indexer/fossology_license.py | ||
|---|---|---|
| 171–172 | Quite! | |
| swh/indexer/tests/test_fossology_license.py | ||
| 51–65 | I don't want to call the super() because there are issues with it (loading configuration from disk for one). | |
| swh/indexer/tests/test_utils.py | ||
| 595 | Yes, because the method tested has one. | |
I disagree with your systematic 'require changes' policy approach.
I do it so that Phabricator does not put it in my "Ready to Review" queue.