Page MenuHomeSoftware Heritage

loader.mercurial: Increase coverage
ClosedPublic

Authored by ardumont on Dec 11 2018, 8:01 PM.

Diff Detail

Repository
rDLDHG Mercurial 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 created this revision.Dec 11 2018, 8:01 PM
ardumont marked an inline comment as done.Dec 11 2018, 8:04 PM
ardumont added inline comments.
swh/loader/mercurial/loader.py
61 ↗(On Diff #2555)

because it's unused, also not captured in tests [1]
I'm not sure i understand fully how it works so...

[1] https://forge.softwareheritage.org/source/puppet-swh-site/browse/production/data/defaults.yaml$1847

ardumont updated this revision to Diff 2556.Dec 11 2018, 9:39 PM
  • test_loader_verifier: Simplify test initialization
  • loader.mercurial: Add the mercurial archive loader scenario
  • loader.mercurial: Add the mercurial archive loader failure scenario
ardumont edited the summary of this revision. (Show Details)Dec 11 2018, 9:40 PM
ardumont edited the summary of this revision. (Show Details)Dec 12 2018, 10:49 AM
vlorentz accepted this revision.Dec 12 2018, 11:56 AM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/loader/mercurial/loader_verifier.py
186–210

Shouldn't we query the storage instead?

This revision is now accepted and ready to land.Dec 12 2018, 11:56 AM
anlambert accepted this revision.Dec 12 2018, 12:01 PM
anlambert added a subscriber: anlambert.

Apart a couple of nitpicks, looks good to me.

swh/loader/mercurial/loader_verifier.py
163

This comment is strange.

swh/loader/mercurial/tests/test_loader.py
193 ↗(On Diff #2556)

Small nitpick, I would rename 'LoaderITest2' into 'LoaderTest2' here and do the same for all test class names.
I don't see the point of that extra 'I' character.

ardumont marked 3 inline comments as done.Dec 12 2018, 12:09 PM
ardumont added inline comments.
swh/loader/mercurial/loader_verifier.py
163

Typo!

186–210

I do not think so.
As we do not call the loader's load function, because of history reasons i suppose [1].
We want to call the inner entrypoints here (I think).

Then again, I only ported the existing code.
In the long run, that might be rewritten or completely dropped.

[1] IIRC, when i took other the loader mercurial, that did not work properly with the storage for example.

swh/loader/mercurial/tests/test_loader.py
193 ↗(On Diff #2556)

Right.

The I stands for integration.

ardumont updated this revision to Diff 2557.Dec 12 2018, 12:11 PM
  • test_loader_verifier: Fix typo
  • tests: Rename ITest to Test classes
This revision was automatically updated to reflect the committed changes.