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

ardumont added inline comments.
swh/loader/git/tests/test_loader.py
137

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
11

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

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

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

This revision now requires changes to proceed.Nov 16 2018, 1:37 PM
vlorentz added inline comments.
swh/loader/git/tests/test_loader.py
137

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

swh/loader/git/tests/test_loader.py
137

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.

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

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

  • Add docstrings explaining quickly test scenarios.
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.