Page MenuHomeSoftware Heritage

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

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

Diff Detail

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

Event Timeline

Remove unneeded tearDown method override call

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
  • 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

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 added inline comments.
swh/loader/pypi/tests/test_loader.py
34

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

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.

  • pypi.tests: No need for extra override cleanup
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

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 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 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
This revision was automatically updated to reflect the committed changes.