Page MenuHomeSoftware Heritage

tests: Use in-memory storage instead of mocking it
ClosedPublic

Authored by anlambert on Tue, Dec 4, 1:39 PM.

Diff Detail

Repository
rDLDPY PyPI loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.Tue, Dec 4, 1:39 PM
vlorentz accepted this revision.Tue, Dec 4, 2:21 PM
vlorentz added inline comments.
swh/loader/pypi/tests/test_loader.py
61

also override it (see D684)

This revision is now accepted and ready to land.Tue, Dec 4, 2:21 PM
anlambert planned changes to this revision.Tue, Dec 4, 4:27 PM
anlambert updated this revision to Diff 2421.Tue, Dec 4, 4:38 PM

Update: prefer to override parse_config_file method for plugging tests config

This revision is now accepted and ready to land.Tue, Dec 4, 4:38 PM

Jenkins build currently fails as a new swh-storage tag must be created and pushed.

Jenkins build currently fails as a new swh-storage tag must be created and pushed.

You can actually tag and push if you need it.
The upload to pypi job should trigger itself when you push the tag.

I'm not sure about the associated debian package though. Maybe it triggers itself also?

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

If you do this, you actually read the configuration from disk (so that can actually diverge from dev box if there is some locally).
I'd say avoid this call altogether (and use a test configuration variable).

anlambert marked an inline comment as done.Tue, Dec 4, 9:52 PM
anlambert added inline comments.
swh/loader/pypi/tests/test_loader.py
69

Indeed you are right, we can have config loaded from files proceeding this way and welcome the side effects.

What I am trying to achieve here is to load the default config without duplicating it in the tests code. I came with this other solution that loads it without reading config files on disk.

from swh.core.config import (
    merge_default_configs, read as config_read,
    SWH_DEFAULT_GLOBAL_CONFIG
)

def parse_config_file(self, *args, **kwargs):
        default_conf = merge_default_configs(self.DEFAULT_CONFIG, self.ADDITIONAL_CONFIG, SWH_DEFAULT_GLOBAL_CONFIG)
        config = config_read(default_conf=default_conf)
        config['storage'] = {'cls': 'memory', 'args': {}}
        return config

But apparently, the plan is to remove implicit configuration (T1410) and thus, based on my understanding, relies only on configuration files. So I am a little confused on what is the proper solution here.

vlorentz added inline comments.Tue, Dec 4, 10:46 PM
swh/loader/pypi/tests/test_loader.py
69

Copy the default config to some global/constant, and return it

ardumont added inline comments.
swh/loader/pypi/tests/test_loader.py
69

Yes, for now, do as proposed by vlorentz.
That will be only duplicated once in the tests and for the tests, that's not too bad considering the alternative.

I'd say avoid this call altogether (and use a test configuration variable).

What i mean was, keep your override implementation of parse_config_file but remove the call to super().parse_config_file.
And the returned result is some variables defining the loader's test configuration.

Something like D770#inline-4166 would do.

But apparently, the plan is to remove implicit configuration (T1410) and thus, based on my understanding, relies only on configuration files

I think the plan will be to remove implicit configuration by explicitely passing configuration along where it's needed.

So that may simplify this anyway but now we will know where to look for changes ;)

anlambert marked an inline comment as done.Wed, Dec 5, 10:55 AM
anlambert added inline comments.
swh/loader/pypi/tests/test_loader.py
69

All right, I will do as proposed.

But I still do not like that configuration duplication, this can be a source of problem
because when it will evolve (new conf key added for instance), we will also have to
synchronize it for the tests. I would prefer to have a mechanism to get the
default configuration and only override relevant conf keys for the tests.

For instance, for the pypi loader, the configuration is the following:

{
 'cache': False,
 'cache_dir': '',
 'content_packet_size': 10000,
 'content_packet_size_bytes': 104857600,
 'content_size_limit': 104857600,
 'debug': False,
 'directory_packet_size': 25000,
 'log_db': 'dbname=softwareheritage-log',
 'occurrence_packet_size': 100000,
 'release_packet_size': 100000,
 'revision_packet_size': 100000,
 'save_data': False,
 'save_data_path': '',
 'send_contents': True,
 'send_directories': True,
 'send_releases': True,
 'send_revisions': True,
 'send_snapshot': True,
 'storage': {'args': {}, 'cls': 'memory'},
 'temp_directory': '/tmp/swh.loader.pypi/'
}

That's quite a lot of configuration keys, with some coming from the swh-core module
and the others from the loader ones (core and pypi). They are not easy to get
apart using the code snippet proposed above.
Plus, the only overridden conf variable for the tests is the storage one.

ardumont added inline comments.Wed, Dec 5, 11:19 AM
swh/loader/pypi/tests/test_loader.py
69

I agree. It's far from perfect. I do not like it either. It's a momentary trade-off.

I like it better than the current side-effect of loading from disk.
I've got quite often being side-tracked by tests passing (or not) because of that side-effect.

Note that the config from swh-core has not being changed for a while now (it still could but it has not for a long time, at least 2 years IIRC ;).

anlambert updated this revision to Diff 2438.Wed, Dec 5, 11:48 AM

Update:

  • use a test configuration dict instead of calling super().parse_config_file
  • bump required version of swh-storage and swh-loader-core
  • use new test method names from swh-loader-core
This revision was automatically updated to reflect the committed changes.