Page MenuHomeSoftware Heritage

Run git loader tests on BulkUpdater too.
ClosedPublic

Authored by vlorentz on Nov 16 2018, 12:26 PM.

Details

Diff Detail

Repository
rDLDG Git loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Nov 16 2018, 12:26 PM
vlorentz updated this revision to Diff 2094.Nov 16 2018, 1:13 PM
  • Rebase
ardumont added inline comments.
swh/loader/git/tests/test_loader.py
138

It'd be good to explain this.

Those values are returned to simulate archive cache hit.
Thus the git updater's update behavior kicks in.

(i think)

swh/loader/git/tests/test_updater.py
12

Same a docstring explaining what scenario is tested would be good.

ardumont added inline comments.Nov 16 2018, 1:27 PM
swh/loader/git/tests/test_updater.py
12

(Ideally, we should have added that in the other test class as well but out of scope here, just mentioning it)

ardumont requested changes to this revision.Nov 16 2018, 1:37 PM
This revision now requires changes to proceed.Nov 16 2018, 1:37 PM
vlorentz marked an inline comment as done.Nov 16 2018, 1:43 PM
vlorentz added inline comments.
swh/loader/git/tests/test_loader.py
138

What do you mean? It has the exact same behavior as Storage.object_find_by_sha1_git (other than operating on mock data)

ardumont added inline comments.Nov 16 2018, 2:29 PM
swh/loader/git/tests/test_loader.py
138

I mean as there is is no context in that diff, it's not apparent that this diff actually test anything.
(It does, i understood it but not immediately).

In general, having context helps.
Maybe only adding those comments on the main test class below is enough.

vlorentz marked 3 inline comments as done.Nov 16 2018, 2:34 PM
vlorentz updated this revision to Diff 2096.Nov 16 2018, 2:35 PM
  • Document test classes.
ardumont added inline comments.Nov 16 2018, 2:51 PM
swh/loader/git/tests/test_updater.py
12

That's not helpful. There is no docstring in the DirGitLoaderTest class which explains anything.

I know it's boring but it's useful.
To understand what the diff (at this very moment) and the tests (in the long run) are all about.

To have an example, check the loader-svn's tests [1] or the loader-mercurial's [2]

[1] https://forge.softwareheritage.org/source/swh-loader-svn/browse/master/swh/loader/svn/tests/test_loader.py

[2] https://forge.softwareheritage.org/source/swh-loader-mercurial/browse/master/swh/loader/mercurial/tests/test_loader.py

vlorentz updated this revision to Diff 2100.Nov 16 2018, 3:08 PM
  • Add docstrings explaining quickly test scenarios.
ardumont accepted this revision.Nov 16 2018, 3:38 PM

Awesome.

This revision is now accepted and ready to land.Nov 16 2018, 3:38 PM
This revision was automatically updated to reflect the committed changes.