Page MenuHomeSoftware Heritage
Differential D3040

Add a split_content argument to object_dicts() and objects() strategies
ClosedPublic
Actions

Authored by douardda on Apr 21 2020, 2:54 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Commits
rDMOD1037e88d9738: Add a split_content argument to object_dicts() and objects() strategies
Summary

Make it possible to generate Content and SkippedContent under different
object types (namely "content" and "skipped_content").

Default to False to keep backward compat.

Depends on D3037.

Diff Detail

Repository
rDMOD Data model
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

douardda created this revision.Apr 21 2020, 2:54 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 21 2020, 2:54 PM
swh-public-ci added a comment.Apr 21 2020, 2:55 PM

Build is green

Patch application report for D3040 (id=10801)

Could not rebase; Attempt merge onto bfba3bdda5...

Updating bfba3bd..1037e88
Fast-forward
 swh/model/hypothesis_strategies.py            | 79 +++++++++++++++++++--------
 swh/model/tests/test_hypothesis_strategies.py | 52 ++++++++++++++++--
 2 files changed, 105 insertions(+), 26 deletions(-)
Changes applied before test
commit 1037e88d97384c4c9b87d37d8b2d3dafc0e821d6
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Apr 21 14:49:14 2020 +0200

    Add a split_content argument to object_dicts() and objects() strategies
    
    Make it possible to generate Content and SkippedContent under different
    object types (namely "content" and "skipped_content").
    
    Default to False to keep backward compat.

commit ebd38077962687e0f962d1edab275f6d33b72312
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Apr 21 11:33:32 2020 +0200

    Add a blacklist_types argument to object_dicts() and objects() hypothesis strategies
    
    so one can choose not to generate some of the object types.
    
    Blacklist "origin_visit_status" by default to prevent breaking dependent
    packages' tests.

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/46/ for more details.

Harbormaster completed remote builds in B11950: Diff 10801.Apr 21 2020, 2:55 PM
ardumont added a subscriber: ardumont.Apr 21 2020, 3:02 PM

Technically, this sounds fine ;)

What is the new use case? Will we merge back the content topics into one or something?

ardumont accepted this revision.Apr 21 2020, 3:43 PM
This revision is now accepted and ready to land.Apr 21 2020, 3:43 PM
Closed by commit rDMOD1037e88d9738: Add a split_content argument to object_dicts() and objects() strategies (authored by douardda). · Explain WhyApr 21 2020, 4:18 PM
This revision was automatically updated to reflect the committed changes.
douardda added a commit: rDMOD1037e88d9738: Add a split_content argument to object_dicts() and objects() strategies.
douardda added a comment.EditedApr 23 2020, 9:47 AM
In D3040#73644, @ardumont wrote:

Technically, this sounds fine ;)

What is the new use case? Will we merge back the content topics into one or something?

This is needed for D3044 (all this is related to T2355). The idea is when this current stack in journal is tagged, then I can tag D3044, then I can remove all the replayer related code from the journal.
[oops]

Revision Contents
Changeset List

PathSize
swh/
model/
65 lines
tests/
34 lines

Diff 10806

View Options

swh/model/hypothesis_strategies.py

# Copyright (C) 2018 The Software Heritage developers # Copyright (C) 2018 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import os.path import os.path
import zipfile import zipfile
import tempfile import tempfile
import datetime
import subprocess import subprocess
from swh.loader.git.loader import GitLoader, GitLoaderFromArchive from swh.loader.git.loader import GitLoader, GitLoaderFromArchive
from swh.loader.core.tests import BaseLoaderTest, LoaderNoStorage from swh.loader.core.tests import BaseLoaderTest
from swh.model.hashutil import hash_to_bytes, hash_to_bytehex
class MockStorage0: class GitLoaderFromArchive(GitLoaderFromArchive):
"""The storage's state before anything is added.""" def project_name_from_archive(self, archive_path):
def snapshot_get_latest(self, origin_id): # We don't want the project name to be 'resources'.
return None return 'testrepo'
def content_missing(self, contents, key_hash='sha1'):
return [c[key_hash] for c in contents]
def directory_missing(self, directories):
return directories
def revision_missing(self, revisions):
return revisions
def object_find_by_sha1_git(self, ids):
return {}
CONTENT1 = { CONTENT1 = {
'33ab5639bfd8e7b95eb1d8d0b87781d4ffea4d5d', # README v1 '33ab5639bfd8e7b95eb1d8d0b87781d4ffea4d5d', # README v1
'349c4ff7d21f1ec0eda26f3d9284c293e3425417', # README v2 '349c4ff7d21f1ec0eda26f3d9284c293e3425417', # README v2
'799c11e348d39f1704022b8354502e2f81f3c037', # file1.txt '799c11e348d39f1704022b8354502e2f81f3c037', # file1.txt
'4bdb40dfd6ec75cb730e678b5d7786e30170c5fb', # file2.txt '4bdb40dfd6ec75cb730e678b5d7786e30170c5fb', # file2.txt
} }
Show All 25 Lines 'branches': {
}, },
'refs/tags/branch2-before-delete': { 'refs/tags/branch2-before-delete': {
'target': '1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b', 'target': '1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b',
'target_type': 'revision', 'target_type': 'revision',
}, },
}, },
} }
SUBDIR_HASH = hash_to_bytes('d53f143d5f3aadb278aad60c4e9a17945a2d68de')
# directory hashes obtained with: # directory hashes obtained with:
# gco b6f40292c4e94a8f7e7b4aff50e6c7429ab98e2a # gco b6f40292c4e94a8f7e7b4aff50e6c7429ab98e2a
# swh-hashtree --ignore '.git' --path . # swh-hashtree --ignore '.git' --path .
# gco 2f01f5ca7e391a2f08905990277faf81e709a649 # gco 2f01f5ca7e391a2f08905990277faf81e709a649
# swh-hashtree --ignore '.git' --path . # swh-hashtree --ignore '.git' --path .
# gco bcdc5ebfde1a3cd6c96e0c2ea4eed19c13208777 # gco bcdc5ebfde1a3cd6c96e0c2ea4eed19c13208777
# swh-hashtree --ignore '.git' --path . # swh-hashtree --ignore '.git' --path .
# gco 1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b # gco 1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b
Show All 17 Lines '79f65ac75f79dda6ff03d66e1242702ab67fb51c':
'5df34ec74d6f69072d9a0a6677d8efbed9b12e60', '5df34ec74d6f69072d9a0a6677d8efbed9b12e60',
'b0a77609903f767a2fd3d769904ef9ef68468b87': 'b0a77609903f767a2fd3d769904ef9ef68468b87':
'9ca0c7d6ffa3f9f0de59fd7912e08f11308a1338', '9ca0c7d6ffa3f9f0de59fd7912e08f11308a1338',
'bd746cd1913721b269b395a56a97baf6755151c2': 'bd746cd1913721b269b395a56a97baf6755151c2':
'e1d0d894835f91a0f887a4bc8b16f81feefdfbd5', 'e1d0d894835f91a0f887a4bc8b16f81feefdfbd5',
} }
class MockStorage1:
"""The storage's state after the first snapshot is loaded."""
def snapshot_get_latest(self, origin_id):
# The following line reencodes SNAPSHOT1 from the format expected
# by assertSnapshotOk to the one that Storage.snapshot_get_latest
# returns.
return {
'id': hash_to_bytes(SNAPSHOT1['id']),
'branches': {
branch_name.encode(): {
'target': hash_to_bytes(branch['target']),
'target_type': branch['target_type']}
for (branch_name, branch)
in SNAPSHOT1['branches'].items()}}
def content_missing(self, contents, key_hash='sha1'):
return map(hash_to_bytes,
{c[key_hash] for c in contents} - CONTENT1)
def directory_missing(self, directories):
assert all(isinstance(d, bytes) for d in directories)
return (set(directories) -
set(map(hash_to_bytes, REVISIONS1)) -
{hash_to_bytes(SUBDIR_HASH)})
def revision_missing(self, revisions):
assert all(isinstance(r, bytes) for r in revisions)
return list(set(revisions) - set(map(hash_to_bytes, REVISIONS1)))
def object_find_by_sha1_git(self, ids):
res = {}
for id_ in ids:
found = []
decoded_id = hash_to_bytehex(id_)
if decoded_id in REVISIONS1:
found.append({
'sha1_git': id_,
'type': 'revision',
'id': id_,
'object_id': 42,
})
elif decoded_id in REVISIONS1.values():
found.append({
'sha1_git': id_,
'type': 'directory',
'id': id_,
'object_id': 42,
})
elif decoded_id == SUBDIR_HASH:
found.append({
'sha1_git': id_,
'type': 'directory',
'id': id_,
'object_id': 42,
})
elif decoded_id in CONTENT1:
found.append({
'sha1_git': id_,
'type': 'content',
'id': id_,
'object_id': 42,
})
res[id_] = found
return res
class LoaderNoStorageMixin(LoaderNoStorage):
def __init__(self):
super().__init__()
self.origin_id = 1
self.visit = 1
self.storage = MockStorage0()
class GitLoaderNoStorage(LoaderNoStorageMixin, GitLoader):
pass
class GitLoaderFromArchiveNoStorage(LoaderNoStorageMixin,
GitLoaderFromArchive):
def project_name_from_archive(self, archive_path):
# We don't want the project name to be 'resources'.
return 'testrepo'
class BaseGitLoaderTest(BaseLoaderTest): class BaseGitLoaderTest(BaseLoaderTest):
def setUp(self, archive_name, uncompress_archive, filename='testrepo'): def setUp(self, archive_name, uncompress_archive, filename='testrepo'):
super().setUp(archive_name=archive_name, filename=filename, super().setUp(archive_name=archive_name, filename=filename,
prefix_tmp_folder_name='swh.loader.git.', prefix_tmp_folder_name='swh.loader.git.',
start_path=os.path.dirname(__file__), start_path=os.path.dirname(__file__),
uncompress_archive=uncompress_archive) uncompress_archive=uncompress_archive)
self.origin_id = self.storage.origin_add_one({
'type': 'git',
'url': 'http://example.com/foo.git'})
self.visit = self.storage.origin_visit_add(
self.origin_id, datetime.datetime.utcnow())
class BaseDirGitLoaderTest(BaseGitLoaderTest): class BaseDirGitLoaderTest(BaseGitLoaderTest):
anlambertUnsubmitted
Not Done
Inline Actions

Instead of duplicating base configuration, I would rather use this implementation (see D768)

def parse_config_file(self, *args, **kwargs):
        config = super().parse_config_file(*args, **kwargs)
        config['storage'] = {'cls': 'memory', 'args': {}}
        return config
anlambert: Instead of duplicating base configuration, I would rather use this implementation (see D768)…
ardumontUnsubmitted
Not Done
Inline Actions

I'm the source of disagreement here.
I'd like to avoid the call to the original parse_config_file method here to reduce the disk reading part.
(I've said as much in D768 indeed).

If you take a look at the discussion just below this remark, you'll see the detail ;)

ardumont: I'm the source of disagreement here. I'd like to avoid the call to the original…
"""Mixin base loader test to prepare the git """Mixin base loader test to prepare the git
repository to uncompress, load and test the results. repository to uncompress, load and test the results.
ardumontUnsubmitted
Not Done
Inline Actions

Do you need to?
That's the part reading the disk for configuration file.

So, that could create divergence on dev box if they have some configuration files in the right places ({/etc/softwareheritage/,~/.swh,~/.config/swh}/loader/git-{loader,updater}.yml))

ardumont: Do you need to? That's the part reading the disk for configuration file. So, that could create…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I need a way to obtain content_packet_size and its many friends.

vlorentz: I need a way to obtain `content_packet_size` and its many friends.
ardumontUnsubmitted
Not Done
Inline Actions

In other modules, we duplicate (for now) that information in the TestLoader (TestGitLoader for example).

Side-effects/configuration duplication trade-offs.

ardumont: In other modules, we duplicate (for now) that information in the TestLoader (TestGitLoader for…
This sets up This sets up
""" """
def setUp(self): def setUp(self):
super().setUp('testrepo.tgz', True) super().setUp('testrepo.tgz', True)
self.loader = GitLoaderNoStorage() self.loader = GitLoader()
ardumontUnsubmitted
Not Done
Inline Actions

I do not like doing this...

The GitLoader and the BulkUpdater allows to pass the config dict instead (do not like it either [1] but it's better than mutating the loader instance).

That would also avoid the side effect of loading the configuration file from disk...

[1] because it's been opened solely for the tests...

ardumont: I do not like doing this... The GitLoader and the BulkUpdater allows to pass the config dict…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Agreed. As we talked earlier today, this will need a change to swh.storage.get_storage (D689).

vlorentz: Agreed. As we talked earlier today, this will need a change to `swh.storage.get_storage` (D689).
self.loader.storage = self.storage
def load(self): def load(self):
return self.loader.load( return self.loader.load(
origin_url=self.repo_url, origin_url=self.repo_url,
visit_date='2016-05-03 15:16:32+00', visit_date='2016-05-03 15:16:32+00',
directory=self.destination_path) directory=self.destination_path)
class BaseZipGitLoaderTest(BaseGitLoaderTest): class BaseZipGitLoaderTest(BaseGitLoaderTest):
"""Mixin base loader test to prepare the git """Mixin base loader test to prepare the git
repository to uncompress, load and test the results. repository to uncompress, load and test the results.
This sets up This sets up
""" """
def setUp(self): def setUp(self):
super().setUp('testrepo.tgz', True) super().setUp('testrepo.tgz', True)
self._setup_zip() self._setup_zip()
self.loader = GitLoaderFromArchiveNoStorage() self.loader = GitLoaderFromArchive()
self.loader.storage = self.storage
def _setup_zip(self): def _setup_zip(self):
self._zip_file = tempfile.NamedTemporaryFile('ab', suffix='.zip') self._zip_file = tempfile.NamedTemporaryFile('ab', suffix='.zip')
dest_dir = os.path.normpath(self.destination_path) + '/' dest_dir = os.path.normpath(self.destination_path) + '/'
with zipfile.ZipFile(self._zip_file, 'a') as zip_writer: with zipfile.ZipFile(self._zip_file, 'a') as zip_writer:
for root, dirs, files in os.walk(dest_dir): for root, dirs, files in os.walk(dest_dir):
assert root.startswith(dest_dir) assert root.startswith(dest_dir)
relative_root = os.path.join( relative_root = os.path.join(
Show All 21 Lines
class GitLoaderTests: class GitLoaderTests:
"""Common tests for all git loaders.""" """Common tests for all git loaders."""
def test_load(self): def test_load(self):
"""Loads a simple repository (made available by `setUp()`), """Loads a simple repository (made available by `setUp()`),
and checks everything was added in the storage.""" and checks everything was added in the storage."""
res = self.load() res = self.load()
self.assertEqual(res['status'], 'eventful', res) self.assertEqual(res['status'], 'eventful', res)
self.assertCountContents(4) # two README, file1, file2 self.assertContentsOk(CONTENT1)
vlorentzAuthorUnsubmitted
Done
Inline Actions

Note: that's an unrelated change.

vlorentz: Note: that's an unrelated change.
ardumontUnsubmitted
Not Done
Inline Actions

CONTENT1 is probably badly named then since i see that it's not one content but many ;)

ardumont: CONTENT1 is probably badly named then since i see that it's not one content but many ;)
vlorentzAuthorUnsubmitted
Done
Inline Actions

It's the contents, at "version 1" of the DB (version 0 = before the loader runs, version 1 = after it runs for the first time, version 2 = after it runs for the second time).

vlorentz: It's the contents, at "version 1" of the DB (version 0 = before the loader runs, version 1 =…
ardumontUnsubmitted
Not Done
Inline Actions

CONTENTS_v{0,1,2} etc... would have been more appropriate...
Let's keep this on the side for now.

ardumont: CONTENTS_v{0,1,2} etc... would have been more appropriate... Let's keep this on the side for…
self.assertCountDirectories(7) self.assertCountDirectories(7)
self.assertCountReleases(0) # FIXME: why not 2? self.assertCountReleases(0) # FIXME: why not 2?
self.assertCountRevisions(7) self.assertCountRevisions(7)
self.assertCountSnapshots(1) self.assertCountSnapshots(1)
self.assertRevisionsOk(REVISIONS1) self.assertRevisionsOk(REVISIONS1)
self.assertSnapshotOk(SNAPSHOT1) self.assertSnapshotOk(SNAPSHOT1)
self.assertEqual(self.loader.load_status(), {'status': 'eventful'}) self.assertEqual(self.loader.load_status(), {'status': 'eventful'})
self.assertEqual(self.loader.visit_status(), 'full') self.assertEqual(self.loader.visit_status(), 'full')
def test_load_unchanged(self): def test_load_unchanged(self):
"""Checks loading a repository a second time does not add """Checks loading a repository a second time does not add
any extra data.""" any extra data."""
res = self.load() res = self.load()
self.assertEqual(res['status'], 'eventful') self.assertEqual(res['status'], 'eventful')
self.loader.storage = MockStorage1()
res = self.load() res = self.load()
self.assertEqual(res['status'], 'uneventful') self.assertEqual(res['status'], 'uneventful')
self.assertCountSnapshots(1) self.assertCountSnapshots(1)
class DirGitLoaderTest(BaseDirGitLoaderTest, GitLoaderTests): class DirGitLoaderTest(BaseDirGitLoaderTest, GitLoaderTests):
"""Tests for the GitLoader. Includes the common ones, and """Tests for the GitLoader. Includes the common ones, and
add others that only work with a local dir.""" add others that only work with a local dir."""
▲ Show 20 Lines • Show All 80 Lines • Show Last 20 Lines
View Options

swh/model/tests/test_hypothesis_strategies.py

# Copyright (C) 2018 The Software Heritage developers # Copyright (C) 2018 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import os.path import os.path
import zipfile import zipfile
import tempfile import tempfile
import datetime
import subprocess import subprocess
from swh.loader.git.loader import GitLoader, GitLoaderFromArchive from swh.loader.git.loader import GitLoader, GitLoaderFromArchive
from swh.loader.core.tests import BaseLoaderTest, LoaderNoStorage from swh.loader.core.tests import BaseLoaderTest
from swh.model.hashutil import hash_to_bytes, hash_to_bytehex
class MockStorage0: class GitLoaderFromArchive(GitLoaderFromArchive):
"""The storage's state before anything is added.""" def project_name_from_archive(self, archive_path):
def snapshot_get_latest(self, origin_id): # We don't want the project name to be 'resources'.
return None return 'testrepo'
def content_missing(self, contents, key_hash='sha1'):
return [c[key_hash] for c in contents]
def directory_missing(self, directories):
return directories
def revision_missing(self, revisions):
return revisions
def object_find_by_sha1_git(self, ids):
return {}
CONTENT1 = { CONTENT1 = {
'33ab5639bfd8e7b95eb1d8d0b87781d4ffea4d5d', # README v1 '33ab5639bfd8e7b95eb1d8d0b87781d4ffea4d5d', # README v1
'349c4ff7d21f1ec0eda26f3d9284c293e3425417', # README v2 '349c4ff7d21f1ec0eda26f3d9284c293e3425417', # README v2
'799c11e348d39f1704022b8354502e2f81f3c037', # file1.txt '799c11e348d39f1704022b8354502e2f81f3c037', # file1.txt
'4bdb40dfd6ec75cb730e678b5d7786e30170c5fb', # file2.txt '4bdb40dfd6ec75cb730e678b5d7786e30170c5fb', # file2.txt
} }
Show All 25 Lines 'branches': {
}, },
'refs/tags/branch2-before-delete': { 'refs/tags/branch2-before-delete': {
'target': '1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b', 'target': '1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b',
'target_type': 'revision', 'target_type': 'revision',
}, },
}, },
} }
SUBDIR_HASH = hash_to_bytes('d53f143d5f3aadb278aad60c4e9a17945a2d68de')
# directory hashes obtained with: # directory hashes obtained with:
# gco b6f40292c4e94a8f7e7b4aff50e6c7429ab98e2a # gco b6f40292c4e94a8f7e7b4aff50e6c7429ab98e2a
# swh-hashtree --ignore '.git' --path . # swh-hashtree --ignore '.git' --path .
# gco 2f01f5ca7e391a2f08905990277faf81e709a649 # gco 2f01f5ca7e391a2f08905990277faf81e709a649
# swh-hashtree --ignore '.git' --path . # swh-hashtree --ignore '.git' --path .
# gco bcdc5ebfde1a3cd6c96e0c2ea4eed19c13208777 # gco bcdc5ebfde1a3cd6c96e0c2ea4eed19c13208777
# swh-hashtree --ignore '.git' --path . # swh-hashtree --ignore '.git' --path .
# gco 1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b # gco 1135e94ccf73b5f9bd6ef07b3fa2c5cc60bba69b
Show All 17 Lines '79f65ac75f79dda6ff03d66e1242702ab67fb51c':
'5df34ec74d6f69072d9a0a6677d8efbed9b12e60', '5df34ec74d6f69072d9a0a6677d8efbed9b12e60',
'b0a77609903f767a2fd3d769904ef9ef68468b87': 'b0a77609903f767a2fd3d769904ef9ef68468b87':
'9ca0c7d6ffa3f9f0de59fd7912e08f11308a1338', '9ca0c7d6ffa3f9f0de59fd7912e08f11308a1338',
'bd746cd1913721b269b395a56a97baf6755151c2': 'bd746cd1913721b269b395a56a97baf6755151c2':
'e1d0d894835f91a0f887a4bc8b16f81feefdfbd5', 'e1d0d894835f91a0f887a4bc8b16f81feefdfbd5',
} }
class MockStorage1:
"""The storage's state after the first snapshot is loaded."""
def snapshot_get_latest(self, origin_id):
# The following line reencodes SNAPSHOT1 from the format expected
# by assertSnapshotOk to the one that Storage.snapshot_get_latest
# returns.
return {
'id': hash_to_bytes(SNAPSHOT1['id']),
'branches': {
branch_name.encode(): {
'target': hash_to_bytes(branch['target']),
'target_type': branch['target_type']}
for (branch_name, branch)
in SNAPSHOT1['branches'].items()}}
def content_missing(self, contents, key_hash='sha1'):
return map(hash_to_bytes,
{c[key_hash] for c in contents} - CONTENT1)
def directory_missing(self, directories):
assert all(isinstance(d, bytes) for d in directories)
return (set(directories) -
set(map(hash_to_bytes, REVISIONS1)) -
{hash_to_bytes(SUBDIR_HASH)})
def revision_missing(self, revisions):
assert all(isinstance(r, bytes) for r in revisions)
return list(set(revisions) - set(map(hash_to_bytes, REVISIONS1)))
def object_find_by_sha1_git(self, ids):
res = {}
for id_ in ids:
found = []
decoded_id = hash_to_bytehex(id_)
if decoded_id in REVISIONS1:
found.append({
'sha1_git': id_,
'type': 'revision',
'id': id_,
'object_id': 42,
})
elif decoded_id in REVISIONS1.values():
found.append({
'sha1_git': id_,
'type': 'directory',
'id': id_,
'object_id': 42,
})
elif decoded_id == SUBDIR_HASH:
found.append({
'sha1_git': id_,
'type': 'directory',
'id': id_,
'object_id': 42,
})
elif decoded_id in CONTENT1:
found.append({
'sha1_git': id_,
'type': 'content',
'id': id_,
'object_id': 42,
})
res[id_] = found
return res
class LoaderNoStorageMixin(LoaderNoStorage):
def __init__(self):
super().__init__()
self.origin_id = 1
self.visit = 1
self.storage = MockStorage0()
class GitLoaderNoStorage(LoaderNoStorageMixin, GitLoader):
pass
class GitLoaderFromArchiveNoStorage(LoaderNoStorageMixin,
GitLoaderFromArchive):
def project_name_from_archive(self, archive_path):
# We don't want the project name to be 'resources'.
return 'testrepo'
class BaseGitLoaderTest(BaseLoaderTest): class BaseGitLoaderTest(BaseLoaderTest):
def setUp(self, archive_name, uncompress_archive, filename='testrepo'): def setUp(self, archive_name, uncompress_archive, filename='testrepo'):
super().setUp(archive_name=archive_name, filename=filename, super().setUp(archive_name=archive_name, filename=filename,
prefix_tmp_folder_name='swh.loader.git.', prefix_tmp_folder_name='swh.loader.git.',
start_path=os.path.dirname(__file__), start_path=os.path.dirname(__file__),
uncompress_archive=uncompress_archive) uncompress_archive=uncompress_archive)
self.origin_id = self.storage.origin_add_one({
'type': 'git',
'url': 'http://example.com/foo.git'})
self.visit = self.storage.origin_visit_add(
self.origin_id, datetime.datetime.utcnow())
class BaseDirGitLoaderTest(BaseGitLoaderTest): class BaseDirGitLoaderTest(BaseGitLoaderTest):
anlambertUnsubmitted
Not Done
Inline Actions

Instead of duplicating base configuration, I would rather use this implementation (see D768)

def parse_config_file(self, *args, **kwargs):
        config = super().parse_config_file(*args, **kwargs)
        config['storage'] = {'cls': 'memory', 'args': {}}
        return config
anlambert: Instead of duplicating base configuration, I would rather use this implementation (see D768)…
ardumontUnsubmitted
Not Done
Inline Actions

I'm the source of disagreement here.
I'd like to avoid the call to the original parse_config_file method here to reduce the disk reading part.
(I've said as much in D768 indeed).

If you take a look at the discussion just below this remark, you'll see the detail ;)

ardumont: I'm the source of disagreement here. I'd like to avoid the call to the original…
"""Mixin base loader test to prepare the git """Mixin base loader test to prepare the git
repository to uncompress, load and test the results. repository to uncompress, load and test the results.
ardumontUnsubmitted
Not Done
Inline Actions

Do you need to?
That's the part reading the disk for configuration file.

So, that could create divergence on dev box if they have some configuration files in the right places ({/etc/softwareheritage/,~/.swh,~/.config/swh}/loader/git-{loader,updater}.yml))

ardumont: Do you need to? That's the part reading the disk for configuration file. So, that could create…
vlorentzAuthorUnsubmitted
Done
Inline Actions

I need a way to obtain content_packet_size and its many friends.

vlorentz: I need a way to obtain `content_packet_size` and its many friends.
ardumontUnsubmitted
Not Done
Inline Actions

In other modules, we duplicate (for now) that information in the TestLoader (TestGitLoader for example).

Side-effects/configuration duplication trade-offs.

ardumont: In other modules, we duplicate (for now) that information in the TestLoader (TestGitLoader for…
This sets up This sets up
""" """
def setUp(self): def setUp(self):
super().setUp('testrepo.tgz', True) super().setUp('testrepo.tgz', True)
self.loader = GitLoaderNoStorage() self.loader = GitLoader()
ardumontUnsubmitted
Not Done
Inline Actions

I do not like doing this...

The GitLoader and the BulkUpdater allows to pass the config dict instead (do not like it either [1] but it's better than mutating the loader instance).

That would also avoid the side effect of loading the configuration file from disk...

[1] because it's been opened solely for the tests...

ardumont: I do not like doing this... The GitLoader and the BulkUpdater allows to pass the config dict…
vlorentzAuthorUnsubmitted
Done
Inline Actions

Agreed. As we talked earlier today, this will need a change to swh.storage.get_storage (D689).

vlorentz: Agreed. As we talked earlier today, this will need a change to `swh.storage.get_storage` (D689).
self.loader.storage = self.storage
def load(self): def load(self):
return self.loader.load( return self.loader.load(
origin_url=self.repo_url, origin_url=self.repo_url,
visit_date='2016-05-03 15:16:32+00', visit_date='2016-05-03 15:16:32+00',
directory=self.destination_path) directory=self.destination_path)
class BaseZipGitLoaderTest(BaseGitLoaderTest): class BaseZipGitLoaderTest(BaseGitLoaderTest):
"""Mixin base loader test to prepare the git """Mixin base loader test to prepare the git
repository to uncompress, load and test the results. repository to uncompress, load and test the results.
This sets up This sets up
""" """
def setUp(self): def setUp(self):
super().setUp('testrepo.tgz', True) super().setUp('testrepo.tgz', True)
self._setup_zip() self._setup_zip()
self.loader = GitLoaderFromArchiveNoStorage() self.loader = GitLoaderFromArchive()
self.loader.storage = self.storage
def _setup_zip(self): def _setup_zip(self):
self._zip_file = tempfile.NamedTemporaryFile('ab', suffix='.zip') self._zip_file = tempfile.NamedTemporaryFile('ab', suffix='.zip')
dest_dir = os.path.normpath(self.destination_path) + '/' dest_dir = os.path.normpath(self.destination_path) + '/'
with zipfile.ZipFile(self._zip_file, 'a') as zip_writer: with zipfile.ZipFile(self._zip_file, 'a') as zip_writer:
for root, dirs, files in os.walk(dest_dir): for root, dirs, files in os.walk(dest_dir):
assert root.startswith(dest_dir) assert root.startswith(dest_dir)
relative_root = os.path.join( relative_root = os.path.join(
Show All 21 Lines
class GitLoaderTests: class GitLoaderTests:
"""Common tests for all git loaders.""" """Common tests for all git loaders."""
def test_load(self): def test_load(self):
"""Loads a simple repository (made available by `setUp()`), """Loads a simple repository (made available by `setUp()`),
and checks everything was added in the storage.""" and checks everything was added in the storage."""
res = self.load() res = self.load()
self.assertEqual(res['status'], 'eventful', res) self.assertEqual(res['status'], 'eventful', res)
self.assertCountContents(4) # two README, file1, file2 self.assertContentsOk(CONTENT1)
vlorentzAuthorUnsubmitted
Done
Inline Actions

Note: that's an unrelated change.

vlorentz: Note: that's an unrelated change.
ardumontUnsubmitted
Not Done
Inline Actions

CONTENT1 is probably badly named then since i see that it's not one content but many ;)

ardumont: CONTENT1 is probably badly named then since i see that it's not one content but many ;)
vlorentzAuthorUnsubmitted
Done
Inline Actions

It's the contents, at "version 1" of the DB (version 0 = before the loader runs, version 1 = after it runs for the first time, version 2 = after it runs for the second time).

vlorentz: It's the contents, at "version 1" of the DB (version 0 = before the loader runs, version 1 =…
ardumontUnsubmitted
Not Done
Inline Actions

CONTENTS_v{0,1,2} etc... would have been more appropriate...
Let's keep this on the side for now.

ardumont: CONTENTS_v{0,1,2} etc... would have been more appropriate... Let's keep this on the side for…
self.assertCountDirectories(7) self.assertCountDirectories(7)
self.assertCountReleases(0) # FIXME: why not 2? self.assertCountReleases(0) # FIXME: why not 2?
self.assertCountRevisions(7) self.assertCountRevisions(7)
self.assertCountSnapshots(1) self.assertCountSnapshots(1)
self.assertRevisionsOk(REVISIONS1) self.assertRevisionsOk(REVISIONS1)
self.assertSnapshotOk(SNAPSHOT1) self.assertSnapshotOk(SNAPSHOT1)
self.assertEqual(self.loader.load_status(), {'status': 'eventful'}) self.assertEqual(self.loader.load_status(), {'status': 'eventful'})
self.assertEqual(self.loader.visit_status(), 'full') self.assertEqual(self.loader.visit_status(), 'full')
def test_load_unchanged(self): def test_load_unchanged(self):
"""Checks loading a repository a second time does not add """Checks loading a repository a second time does not add
any extra data.""" any extra data."""
res = self.load() res = self.load()
self.assertEqual(res['status'], 'eventful') self.assertEqual(res['status'], 'eventful')
self.loader.storage = MockStorage1()
res = self.load() res = self.load()
self.assertEqual(res['status'], 'uneventful') self.assertEqual(res['status'], 'uneventful')
self.assertCountSnapshots(1) self.assertCountSnapshots(1)
class DirGitLoaderTest(BaseDirGitLoaderTest, GitLoaderTests): class DirGitLoaderTest(BaseDirGitLoaderTest, GitLoaderTests):
"""Tests for the GitLoader. Includes the common ones, and """Tests for the GitLoader. Includes the common ones, and
add others that only work with a local dir.""" add others that only work with a local dir."""
▲ Show 20 Lines • Show All 80 Lines • Show Last 20 Lines
About · Developer information · Legal