converters: now check for number of parents and adding this information to the revision
api, browse: removal of the revision log 'limit'. The browsing view displays information
about a revision if it is a merge, so the user is warned some commits might not show
Details
- Reviewers
zack - Group Reviewers
Reviewers - Maniphest Tasks
- T258: /api/revision/<sha1_git>/log/ uses a limit which should not exist
- Commits
- R65:5d895d0d18dd: Service tests: factorize test data structures
R65:bbe604cc348c: renderers: new jinja2 filter for revision log
R65:2cd5e8227e0a: Remove revision log 'limit'
R65:9c862dff0772: Add merge detection to revision information
R65:4c7a776bb92e: tests: factorize and cleanup
rDWAPPS4c7a776bb92e: tests: factorize and cleanup
rDWAPPS5d895d0d18dd: Service tests: factorize test data structures
rDWAPPSbbe604cc348c: renderers: new jinja2 filter for revision log
rDWAPPS2cd5e8227e0a: Remove revision log 'limit'
rDWAPPS9c862dff0772: Add merge detection to revision information
Diff Detail
- Repository
- rDWAPPS Web applications
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Two classes of comments about this diff:
- minor stylistic stuff at the beginning
- lotsa copy/paste canned values in the tests, that should really be factored out before accepting this
When addressing (2), please use as constant/static attribute names something that makes it obvious what they are used for in the tests, e.g., EXPECTED_REVISION, NON_EXISTENT_REVISION, ROBOT_COMMITTER, etc. That practice makes tests much more readable and maintainable in the long run.
TIA!
swh/web/ui/renderers.py | ||
---|---|---|
96 ↗ | (On Diff #282) | typo here "do" -> "to" |
99 ↗ | (On Diff #282) | stylistic matter: the src/dest variables are used only once (hence no speed up in using them), and naming them doesn't add anything to the readability of the code. It is minor point, in cases like this one putting the regexps directly in the sub() arguments would be preferrable |
swh/web/ui/templates/revision-log.html | ||
4 | extra empty line here? or is it intended? | |
24 | same here: extra empty line? | |
138 | same for the 3 empty lines here: are they intended? if so (for readability?), leave them around, otherwise please remove them | |
swh/web/ui/tests/test_converters.py | ||
359–370 | several constants here seem to be reused in multiple tests, at least:
please define them either as module-level constants, or as class-level attributes of the test module, and reference them from the various tests where they are used (using self.* or not, depending on the choice) | |
493–498 | details about this artifact look like other candidates for defining constants/attributes, and referencing them from multiple tests | |
swh/web/ui/tests/test_service.py | ||
1304–1313 | here's another one: this revision, or at least many metadata about it, are reused in multiple tests. Please factorize this out, … and DRY (Don't Repeat Yourself) :-) | |
1824–1832 | other stuff to be factored out |
swh/web/ui/tests/test_converters.py | ||
---|---|---|
359–370 | I'd rather remove these entirely than define them as constants, since they're irrelevant to the tests I'm performing. Does that seem fair? | |
493–498 | Same here. | |
swh/web/ui/tests/test_service.py | ||
1304–1313 | I should make a standalone task for this, since there are a lot of other objects that would benefit from factorizing in test_service.py. On the other hand, I believe @ardumont's rationale behind avoiding factorizing was that it much improves readability of the test itself. Since performance is far from being an issue in these tests, I'm tempted to argue against factorizing. |
swh/web/ui/tests/test_service.py | ||
---|---|---|
1304–1313 | Nothing about my comments is about performance. They're all about readability/maintanability of the tests. I'm not against a separate task for fixing this for old code, but your proposed change is contributing to the problem. So please avoid introducing even more test value duplication with this change, and submit a separate task for the old code. (I think it'd be easier to just fix this all at once, but YMMV.) |
I suggest looking at swh.storage for one way of keeping the test data "factored out".
- tests: factorize and cleanup
- renderers: fix typo
- revision log template: remove empty lines
Almost there. Deduplication is great, you just need to push it further, to also include dictionary structures :-)
swh/web/ui/tests/test_converters.py | ||
---|---|---|
359–370 | Sure. Just note it down as low priority task, maybe? | |
swh/web/ui/tests/test_service.py | ||
985–986 | OK, almost there! The constants you added are great and reduce duplication a lot. I get there are two versions of it: one with _BIN/_RAW, the other with strings, but they are just 2 which is better than 10. mock_backend.revision_get = MagicMock(return_value=REVISION_RAW) | |
997–1011 | this will become self.assertEqual(actual_revision, REVISION) | |
1019 | this OTOH it is totally fine as is, as you need a finer grained version of the macro-constant | |
1025 | This could remain as is, as you need to do something different for this case. stub_rev = REVISION_RAW stub_rev['id'] = ... stub_rev['message'] = ... which has the added benefit of making clear what is different in the test data of this test case from other ones. (Note: the above has potentially variable aliasing issues though, so you should either use a copy constructor, or make sure the constants are in a place that get refreshed for each test case.) | |
1066 | etc. |
- Service tests: factorize test data structures
I finally get what you're saying. Thanks for putting up with the iterations ^^
swh/web/ui/tests/test_service.py | ||
---|---|---|
1304–1313 |
@jbertran Precision: avoiding factorizing in a test context. I do abide by the DRY principle but not fully in a test context. We have too much duplication now. Regarding this factorization, we could also create some intermediary helper functions make_revision, make_tree (or something) which takes as parameter the important part (in regard of that test, it seems to be the id here). |