Page MenuHomeSoftware Heritage
Differential D475

loader.pypi.tests: Refactor using loader-core mixins
ClosedPublic
Actions

Authored by ardumont on Oct 4 2018, 11:20 AM.

Details

Reviewers
anlambert
olasd
Commits
rDLDPY62527b28d297: .gitignore: Ignore .tox/ folder
rDLDPYce8d2eaf77b2: README.md: Fix typo
rDLDPYbce8c40e1285: pypi.tests: No need for extra override cleanup
rDLDPY5e3afe09c9d4: pypi.tests: Do not leak core loader fixture's state
rDLDPY9a65ff0477a2: swh.loader.pypi.model: Remove dead code
rDLDPYdead01f5207e: pypi.tests: Fix missing cleanup step
rDLDPYe5caa83e81c9: loader.tests: Refactor using loader-core mixins
Summary

Clean up duplication in tests code by reusing the loader-core's
dedicated mixins.

Also:

  • Cleanup swh.loader.pypi.model remnant module
  • Fix typos
  • Fix a missing dangling temporary folder

Related T1238

Depends on D467

Test Plan

make test

Diff Detail

Repository
rDLDPY PyPI loader
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1607
Build 1951: arc lint + arc unit

Event Timeline

ardumont created this revision.Oct 4 2018, 11:20 AM
Harbormaster completed remote builds in B1504: Diff 1441.Oct 4 2018, 11:20 AM
ardumont updated this revision to Diff 1442.Oct 4 2018, 11:27 AM

Remove unneeded tearDown method override call

Harbormaster completed remote builds in B1505: Diff 1442.Oct 4 2018, 11:27 AM
ardumont retitled this revision from loader.tests: Refactor using loader-core mixins to loader.pypi.tests: Refactor using loader-core mixins.Oct 8 2018, 1:59 PM
ardumont updated this revision to Diff 1556.Oct 9 2018, 12:24 PM
  • loader.tests: Refactor using loader-core mixins
  • pypi.tests: Fix missing cleanup step
  • swh.loader.pypi.model: Remove dead code
  • README.md: Fix typo
  • pypi.tests: Do not leak core loader fixture's state

+ Rebase

Harbormaster completed remote builds in B1593: Diff 1556.Oct 9 2018, 12:24 PM
anlambert added a comment.Oct 10 2018, 1:35 PM

Apart the overriding of the cleanup method in TestPyPILoader which I think can be avoided, it looks good to me.

swh/loader/pypi/tests/test_loader.py
34

Here, maybe you could use the temp_directory instance variable from the PyPILoader class instead (https://forge.softwareheritage.org/source/swh-loader-pypi/browse/master/swh/loader/pypi/loader.py$79)?
This way, you don't need to override the cleanup method.

ardumont marked an inline comment as done.Oct 10 2018, 1:39 PM
ardumont added inline comments.
swh/loader/pypi/tests/test_loader.py
34

Not sure, will check.
Thanks for the heads up.

ardumont marked an inline comment as done.Oct 10 2018, 2:00 PM
ardumont added inline comments.
swh/loader/pypi/tests/test_loader.py
34

I'm actually doing that (through the initialized client).
As I'm providing the client when instantiating the PyPIProject, i'm using that client's temp directory.

Hence, when clarifying this, i'm realizing i should not have to override the cleanup...
But somehow i do, i have dangling files right now without the override...

Digging further.

ardumont mentioned this in rDLDBASEc15df60ad5a4: core.tests: Fix: Pass along parameter to constructor.Oct 10 2018, 2:14 PM
ardumont updated this revision to Diff 1576.Oct 10 2018, 2:18 PM
  • pypi.tests: No need for extra override cleanup
Harbormaster completed remote builds in B1607: Diff 1576.Oct 10 2018, 2:18 PM
ardumont marked an inline comment as done.Oct 10 2018, 2:23 PM
ardumont added inline comments.
swh/loader/pypi/tests/test_loader.py
34

Ok, it's a bug in the loader-core's new mixin feature initialization step.

It does not pass along the constructor parameters...
The client thus ends up None instead of the one we instantiate here.
The loader thus goes on building the temporary directory. Then not working with the one i thought it used...

Fix [1] is done there so now i can safely remove the extra cleanup.

[1]rDLDBASEc15df60ad5a404e50469f1fc9dc868916c5e7e2e

anlambert added inline comments.Oct 10 2018, 2:30 PM
swh/loader/pypi/tests/test_loader.py
34

Ok but I still have the feeling that self.temp_dir will not be cleanup.
Based on what I read, PyPIClientWithCache does not remove the temp
directory passed as parameter and the PyPILoader class cleanup the
directory pointed by self.temp_directory.
What am I mising here ?

ardumont marked an inline comment as done.Oct 10 2018, 2:37 PM
ardumont added inline comments.
swh/loader/pypi/tests/test_loader.py
34

Ok but I still have the feeling that self.temp_dir will not be cleanup.

It is.

I forgot to mention that i no longer have dangling files in core and pypi loaders in the current diff state (and commit).

What am i missing here?

self.temp_directory is self.temp_dir as per [1]

[1] https://forge.softwareheritage.org/source/swh-loader-pypi/browse/master/swh/loader/pypi/loader.py$51

anlambert accepted this revision.Oct 10 2018, 2:40 PM
anlambert added inline comments.
swh/loader/pypi/tests/test_loader.py
34

Ok, did not spot the call to super().__init__ below.

This revision is now accepted and ready to land.Oct 10 2018, 2:40 PM
Closed by commit rDLDPYe5caa83e81c9: loader.tests: Refactor using loader-core mixins (authored by ardumont). · Explain WhyOct 10 2018, 2:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
1 line
6 lines
debian/
4 lines
2 lines
swh/
loader/
pypi/
tests/
95 lines
148 lines

Diff 1576

View Options

.gitignore

Loading...
View Options

README.md

Loading...
View Options

debian/control

Loading...
View Options

requirements-swh.txt

Loading...
View Options

swh/loader/pypi/model.py

Loading...
View Options

swh/loader/pypi/tests/common.py

Loading...
View Options

swh/loader/pypi/tests/test_loader.py

Loading...
About · Developer information · Legal