Page MenuHomeSoftware Heritage
Differential D802

loader.mercurial: Increase coverage
ClosedPublic
Actions

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

Details

Reviewers
vlorentz
anlambert
Group Reviewers
Reviewers
Commits
rDLDHG4f89686fdc09: tests: Rename ITest to Test classes
rDLDHG076c5aa959a4: test_loader_verifier: Fix typo
rDLDHG9bb922bef12d: loader.mercurial: Add the mercurial archive loader failure scenario
rDLDHGb0c03268de3d: test_converters: Add coverage on converters
rDLDHGac1e6227d359: loader.mercurial: Add the mercurial archive loader scenario
rDLDHG34c5831bbef4: test_loader_verifier: Simplify test initialization
rDLDHGdd9182a05e13: tests: Add another loader verifier test scenario
rDLDHGca237dc86e2f: tests_tasks: Add scenario to check tasks are fine
rDLDHG19c15e64211e: loader_verifier: Transform the verifier into test
rDLDHG293b7833d254: loader.mercurial: Flip the reduce_effort flag to False
rDLDHG5985c5245a56: loader_verifier: Clear dead comments
Summary

As per my suggestion [1], transformed the loader verifier code into tests.

  • loader_verifier: Transform the verifier into tests
  • tests_tasks: Add scenario to check tasks are fine
  • test_converters: Add coverage on converters
  • Add scenario for archive mercurial loader (success + failure)

[1] https://forge.softwareheritage.org/T1411#26191

Related T1411
Related T1439
Related T1443

Test Plan

tox

Diff Detail

Repository
rDLDHG Mercurial loader
Branch
improve-coverage
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2978
Build 3802: tox-on-jenkinsJenkins
Build 3801: arc lint + arc unit

Event Timeline

ardumont created this revision.Dec 11 2018, 8:01 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptDec 11 2018, 8:01 PM
Harbormaster completed remote builds in B2978: Diff 2555.Dec 11 2018, 8:03 PM
swh-public-ci added a subscriber: swh-public-ci.Dec 11 2018, 8:03 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/19/ for more details.

ardumont marked an inline comment as done.Dec 11 2018, 8:04 PM
ardumont added inline comments.
swh/loader/mercurial/loader.py
61

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
swh-public-ci added a comment.Dec 11 2018, 9:41 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/20/ for more details.

Harbormaster completed remote builds in B2979: Diff 2556.Dec 11 2018, 9:41 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/tests/test_loader_verifier.py
193–218

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/tests/test_loader.py
203

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.

swh/loader/mercurial/tests/test_loader_verifier.py
168

This comment is strange.

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

Right.

The I stands for integration.

swh/loader/mercurial/tests/test_loader_verifier.py
168

Typo!

193–218

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.

ardumont updated this revision to Diff 2557.Dec 12 2018, 12:11 PM
  • test_loader_verifier: Fix typo
  • tests: Rename ITest to Test classes
swh-public-ci added a comment.Dec 12 2018, 12:12 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLDHG/job/tox/21/ for more details.

Harbormaster completed remote builds in B2980: Diff 2557.Dec 12 2018, 12:12 PM
Closed by commit rDLDHG5985c5245a56: loader_verifier: Clear dead comments (authored by ardumont). · Explain WhyDec 12 2018, 1:23 PM
This revision was automatically updated to reflect the committed changes.
ardumont edited the summary of this revision. (Show Details)Dec 14 2018, 8:41 AM
ardumont mentioned this in T1443: Make swh services run within docker and docker-compose.

Revision Contents
Changeset List

PathSize
swh/
loader/
mercurial/
2 lines
tests/
52 lines
76 lines
48 lines
53 lines
tests/
test_loader_verifier.py
loader_verifier.py
128 lines

Diff 2555

View Options

swh/loader/mercurial/loader.py

Loading...
View Options

swh/loader/mercurial/loader_verifier.py

Loading...
View Options

swh/loader/mercurial/tests/common.py

Loading...
View Options

swh/loader/mercurial/tests/test_converters.py

Loading...
View Options

swh/loader/mercurial/tests/test_loader.py

Loading...
View Options

swh/loader/mercurial/tests/test_loader_verifier.py

Loading...
View Options

swh/loader/mercurial/tests/test_tasks.py

Loading...
About · Developer information · Legal