Page MenuHomeSoftware Heritage
Differential D5074

mercurial.loader: Make error comparison case-insensitive
ClosedPublic
Actions

Authored by ardumont on Feb 15 2021, 4:59 PM.

Details

Reviewers
vsellier
Group Reviewers
Reviewers
Commits
rDLDHG71cfd100180f: mercurial.loader: Make error comparison case-insensitive
Summary

This avoids failing visits for the wrong comparison check [1]

[1] https://sentry.softwareheritage.org/share/issue/27017710a5ec49f991910a780d38d4ab/

Diff Detail

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

Event Timeline

ardumont created this revision.Feb 15 2021, 4:59 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 15 2021, 4:59 PM
swh-public-ci added a comment.Feb 15 2021, 5:01 PM

Build is green

Patch application report for D5074 (id=18110)

Rebasing onto 4c84a86b37...

Current branch diff-target is up to date.
Changes applied before test
commit 71cfd100180fcd65e0ef3996fdf6a0e753d5a7f9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Feb 15 16:51:40 2021 +0100

    mercurial.loader: Make error comparison case-insensitive
    
    This avoids failing visits for the wrong comparison check [1]
    
    [1] https://sentry.softwareheritage.org/share/issue/27017710a5ec49f991910a780d38d4ab/

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

Harbormaster completed remote builds in B19218: Diff 18110.Feb 15 2021, 5:01 PM
ardumont requested review of this revision.Feb 15 2021, 5:01 PM
vsellier accepted this revision.Feb 15 2021, 5:22 PM
This revision is now accepted and ready to land.Feb 15 2021, 5:22 PM
Closed by commit rDLDHG71cfd100180f: mercurial.loader: Make error comparison case-insensitive (authored by ardumont). · Explain WhyFeb 15 2021, 5:23 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDHG71cfd100180f: mercurial.loader: Make error comparison case-insensitive.

Revision Contents
Changeset List

PathSize
swh/
loader/
mercurial/
6 lines
tests/
5 lines

Diff 18111

View Options

swh/loader/mercurial/loader.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/loader/mercurial/tests/test_loader.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