Page MenuHomeSoftware Heritage
Differential D87 Diff 292 swh/web/ui/tests/test_service.py

Changeset View

Standalone View

View Options

swh/web/ui/tests/test_service.py

Show First 20 Lines • Show All 568 Lines • ▼ Show 20 Lines def lookup_revision_with_context(self, mock_query, mock_backend):
sha1_git) sha1_git)
# then # then
self.assertEquals(actual_revision, { self.assertEquals(actual_revision, {
'id': hash_to_hex(sha1_git_bin), 'id': hash_to_hex(sha1_git_bin),
'parents': [], 'parents': [],
'children': [hash_to_hex(b'999'), hash_to_hex(b'777')], 'children': [hash_to_hex(b'999'), hash_to_hex(b'777')],
'directory': hash_to_hex(b'278'), 'directory': hash_to_hex(b'278'),
'merge': False
}) })
mock_query.parse_hash_with_algorithms_or_throws.assert_has_calls( mock_query.parse_hash_with_algorithms_or_throws.assert_has_calls(
[call(sha1_git, ['sha1'], 'Only sha1_git is supported.'), [call(sha1_git, ['sha1'], 'Only sha1_git is supported.'),
call(sha1_git_root, ['sha1'], 'Only sha1_git is supported.')]) call(sha1_git_root, ['sha1'], 'Only sha1_git is supported.')])
mock_backend.revision_log.assert_called_with( mock_backend.revision_log.assert_called_with(
sha1_git_root_bin, 100) sha1_git_root_bin, 100)
▲ Show 20 Lines • Show All 56 Lines • ▼ Show 20 Lines def lookup_revision_with_context_sha1_git_root_already_retrieved_as_dict(
sha1_git) sha1_git)
# then # then
self.assertEquals(actual_revision, { self.assertEquals(actual_revision, {
'id': hash_to_hex(sha1_git_bin), 'id': hash_to_hex(sha1_git_bin),
'parents': [], 'parents': [],
'children': [hash_to_hex(b'999'), hash_to_hex(b'777')], 'children': [hash_to_hex(b'999'), hash_to_hex(b'777')],
'directory': hash_to_hex(b'278'), 'directory': hash_to_hex(b'278'),
'merge': False
}) })
mock_query.parse_hash_with_algorithms_or_throws.assert_called_once_with( # noqa mock_query.parse_hash_with_algorithms_or_throws.assert_called_once_with( # noqa
sha1_git, ['sha1'], 'Only sha1_git is supported.') sha1_git, ['sha1'], 'Only sha1_git is supported.')
mock_backend.revision_get.assert_called_once_with(sha1_git_bin) mock_backend.revision_get.assert_called_once_with(sha1_git_bin)
mock_backend.revision_log.assert_called_with( mock_backend.revision_log.assert_called_with(
▲ Show 20 Lines • Show All 318 Lines • ▼ Show 20 Lines def lookup_revision(self, mock_backend):
}, },
'committer_date': { 'committer_date': {
'timestamp': datetime.datetime( 'timestamp': datetime.datetime(
2000, 1, 17, 11, 23, 54, 2000, 1, 17, 11, 23, 54,
tzinfo=datetime.timezone.utc, tzinfo=datetime.timezone.utc,
).timestamp(), ).timestamp(),
'offset': 0, 'offset': 0,
'negative_utc': False, 'negative_utc': False,
}, },
'synthetic': False, 'synthetic': False,
zackUnsubmitted
Not Done
Inline Actions

OK, almost there! The constants you added are great and reduce duplication a lot.
… but now what is duplicated is the above records, with all its 7 fields, which appears in several places :-)

I get there are two versions of it: one with _BIN/_RAW, the other with strings, but they are just 2 which is better than 10.
So please add two additional constants, e.g.: REVISION_RAW, and REVISION, defined using the other more fine grained constants, and in places like this just go with:

mock_backend.revision_get = MagicMock(return_value=REVISION_RAW)
zack: OK, //almost// there! The constants you added are great and reduce duplication a lot. … but now…
'type': 'git', 'type': 'git',
'parents': [], 'parents': [],
'metadata': [], 'metadata': [],
}) })
# when # when
actual_revision = service.lookup_revision( actual_revision = service.lookup_revision(
'18d8be353ed3480476f032475e7c233eff7371d5') '18d8be353ed3480476f032475e7c233eff7371d5')
# then # then
self.assertEqual(actual_revision, { self.assertEqual(actual_revision, {
'id': '18d8be353ed3480476f032475e7c233eff7371d5', 'id': '18d8be353ed3480476f032475e7c233eff7371d5',
'directory': '7834ef7e7c357ce2af928115c6c6a42b7e2a44e6', 'directory': '7834ef7e7c357ce2af928115c6c6a42b7e2a44e6',
'author': { 'author': {
'name': 'bill & boule', 'name': 'bill & boule',
'email': 'bill@boule.org', 'email': 'bill@boule.org',
}, },
'committer': { 'committer': {
'name': 'boule & bill', 'name': 'boule & bill',
'email': 'boule@bill.org', 'email': 'boule@bill.org',
}, },
'message': 'elegant fix for bug 31415957', 'message': 'elegant fix for bug 31415957',
'date': "2000-01-17T11:23:54+00:00", 'date': "2000-01-17T11:23:54+00:00",
'committer_date': "2000-01-17T11:23:54+00:00", 'committer_date': "2000-01-17T11:23:54+00:00",
'synthetic': False, 'synthetic': False,
zackUnsubmitted
Not Done
Inline Actions

this will become

self.assertEqual(actual_revision, REVISION)
zack: this will become ``` self.assertEqual(actual_revision, REVISION) ```
'type': 'git', 'type': 'git',
'parents': [], 'parents': [],
'metadata': [], 'metadata': [],
'merge': False
}) })
mock_backend.revision_get.assert_called_with( mock_backend.revision_get.assert_called_with(
hex_to_hash('18d8be353ed3480476f032475e7c233eff7371d5')) hex_to_hash('18d8be353ed3480476f032475e7c233eff7371d5'))
zackUnsubmitted
Not Done
Inline Actions

this OTOH it is totally fine as is, as you need a finer grained version of the macro-constant

zack: this OTOH it is totally fine as is, as you need a finer grained version of the macro-constant
@patch('swh.web.ui.service.backend') @patch('swh.web.ui.service.backend')
@istest @istest
def lookup_revision_invalid_msg(self, mock_backend): def lookup_revision_invalid_msg(self, mock_backend):
# given # given
stub_rev = { stub_rev = {
zackUnsubmitted
Not Done
Inline Actions

This could remain as is, as you need to do something different for this case.
But it'd be much nicer as something like:

stub_rev = REVISION_RAW
stub_rev['id'] = ...
stub_rev['message'] = ...

which has the added benefit of making clear what is different in the test data of this test case from other ones.

(Note: the above has potentially variable aliasing issues though, so you should either use a copy constructor, or make sure the constants are in a place that get refreshed for each test case.)

zack: This //could// remain as is, as you need to do something different for this case. But it'd be…
'id': hex_to_hash('123456'), 'id': hex_to_hash('123456'),
'directory': hex_to_hash( 'directory': hex_to_hash(
'7834ef7e7c357ce2af928115c6c6a42b7e2a44e6'), '7834ef7e7c357ce2af928115c6c6a42b7e2a44e6'),
'author': { 'author': {
'name': b'bill & boule', 'name': b'bill & boule',
'email': b'bill@boule.org', 'email': b'bill@boule.org',
}, },
'committer': { 'committer': {
Show All 24 Lines def lookup_revision_invalid_msg(self, mock_backend):
} }
mock_backend.revision_get = MagicMock(return_value=stub_rev) mock_backend.revision_get = MagicMock(return_value=stub_rev)
# when # when
actual_revision = service.lookup_revision( actual_revision = service.lookup_revision(
'18d8be353ed3480476f032475e7c233eff7371d5') '18d8be353ed3480476f032475e7c233eff7371d5')
# then # then
self.assertEqual(actual_revision, { self.assertEqual(actual_revision, {
zackUnsubmitted
Not Done
Inline Actions

etc.

zack: etc.
'id': '123456', 'id': '123456',
'directory': '7834ef7e7c357ce2af928115c6c6a42b7e2a44e6', 'directory': '7834ef7e7c357ce2af928115c6c6a42b7e2a44e6',
'author': { 'author': {
'name': 'bill & boule', 'name': 'bill & boule',
'email': 'bill@boule.org', 'email': 'bill@boule.org',
}, },
'committer': { 'committer': {
'name': 'boule & bill', 'name': 'boule & bill',
'email': 'boule@bill.org', 'email': 'boule@bill.org',
}, },
'message': None, 'message': None,
'message_decoding_failed': True, 'message_decoding_failed': True,
'date': "2000-01-17T11:23:54+00:00", 'date': "2000-01-17T11:23:54+00:00",
'committer_date': "2000-01-17T11:23:54+00:00", 'committer_date': "2000-01-17T11:23:54+00:00",
'synthetic': False, 'synthetic': False,
'type': 'git', 'type': 'git',
'parents': [], 'parents': [],
'metadata': [], 'metadata': [],
'merge': False
}) })
mock_backend.revision_get.assert_called_with( mock_backend.revision_get.assert_called_with(
hex_to_hash('18d8be353ed3480476f032475e7c233eff7371d5')) hex_to_hash('18d8be353ed3480476f032475e7c233eff7371d5'))
@patch('swh.web.ui.service.backend') @patch('swh.web.ui.service.backend')
@istest @istest
def lookup_revision_msg_ok(self, mock_backend): def lookup_revision_msg_ok(self, mock_backend):
▲ Show 20 Lines • Show All 202 Lines • ▼ Show 20 Lines def lookup_revision_multiple(self, mock_backend):
'name': 'bill & boule', 'name': 'bill & boule',
'email': 'bill@boule.org', 'email': 'bill@boule.org',
}, },
'committer': { 'committer': {
'name': 'boule & bill', 'name': 'boule & bill',
'email': 'boule@bill.org', 'email': 'boule@bill.org',
}, },
'message': 'elegant fix for bug 31415957', 'message': 'elegant fix for bug 31415957',
'date': '2000-01-17T11:23:54+00:00', 'date': '2000-01-17T11:23:54+00:00',
'date_offset': 0, 'date_offset': 0,
'committer_date': '2000-01-17T11:23:54+00:00', 'committer_date': '2000-01-17T11:23:54+00:00',
'committer_date_offset': 0, 'committer_date_offset': 0,
'synthetic': False, 'synthetic': False,
'type': 'git', 'type': 'git',
'parents': [], 'parents': [],
'metadata': [], 'metadata': [],
'merge': False
}, },
zackUnsubmitted
Not Done
Inline Actions

here's another one: this revision, or at least many metadata about it, are reused in multiple tests. Please factorize this out, … and DRY (Don't Repeat Yourself) :-)

zack: here's another one: this revision, or at least many metadata about it, are reused in multiple…
jbertranAuthorUnsubmitted
Not Done
Inline Actions

I should make a standalone task for this, since there are a lot of other objects that would benefit from factorizing in test_service.py. On the other hand, I believe @ardumont's rationale behind avoiding factorizing was that it much improves readability of the test itself. Since performance is far from being an issue in these tests, I'm tempted to argue against factorizing.

jbertran: I should make a standalone task for this, since there are a lot of other objects that would…
zackUnsubmitted
Not Done
Inline Actions

Nothing about my comments is about performance. They're all about readability/maintanability of the tests.
Having to change test data in 10 difference places instead of 1 is problematic, period. Every time the same thing is written more than once in code, there's a potential problem; when something is written more than two, there's a certain problem :-)
And reading self.COMMIT_ID is much easier and meaningful than reading 18d8be353ed3480476f032475e7c233eff7371d5.

I'm not against a separate task for fixing this for old code, but your proposed change is contributing to the problem. So please avoid introducing even more test value duplication with this change, and submit a separate task for the old code. (I think it'd be easier to just fix this all at once, but YMMV.)

zack: Nothing about my comments is about performance. They're all about readability/maintanability of…
ardumontUnsubmitted
Not Done
Inline Actions

@ardumont's rationale behind avoiding factorizing was that it much improves readability of the test itself...

@jbertran Precision: avoiding factorizing in a test context.

I do abide by the DRY principle but not fully in a test context.
I think now i was simply plain wrong.

We have too much duplication now.

Regarding this factorization, we could also create some intermediary helper functions make_revision, make_tree (or something) which takes as parameter the important part (in regard of that test, it seems to be the id here).

ardumont: > @ardumont's rationale behind avoiding factorizing was that it much improves readability of…
{ {
'id': sha1_other, 'id': sha1_other,
'directory': 'abcdbe353ed3480476f032475e7c233eff7371d5', 'directory': 'abcdbe353ed3480476f032475e7c233eff7371d5',
'author': { 'author': {
'name': 'name', 'name': 'name',
'email': 'name@surname.org', 'email': 'name@surname.org',
}, },
'committer': { 'committer': {
'name': 'name', 'name': 'name',
'email': 'name@surname.org', 'email': 'name@surname.org',
}, },
'message': 'ugly fix for bug 42', 'message': 'ugly fix for bug 42',
'date': '2000-01-12T05:23:54+00:00', 'date': '2000-01-12T05:23:54+00:00',
'date_offset': 0, 'date_offset': 0,
'committer_date': '2000-01-12T05:23:54+00:00', 'committer_date': '2000-01-12T05:23:54+00:00',
'committer_date_offset': 0, 'committer_date_offset': 0,
'synthetic': False, 'synthetic': False,
'type': 'git', 'type': 'git',
'parents': [], 'parents': [],
'metadata': [], 'metadata': [],
'merge': False
} }
]) ])
self.assertEqual( self.assertEqual(
list(mock_backend.revision_get_multiple.call_args[0][0]), list(mock_backend.revision_get_multiple.call_args[0][0]),
[hex_to_hash( [hex_to_hash(
'18d8be353ed3480476f032475e7c233eff7371d5'), '18d8be353ed3480476f032475e7c233eff7371d5'),
hex_to_hash( hex_to_hash(
▲ Show 20 Lines • Show All 79 Lines • ▼ Show 20 Lines def lookup_revision_log(self, mock_backend):
}, },
'message': 'elegant fix for bug 31415957', 'message': 'elegant fix for bug 31415957',
'date': "2000-01-17T11:23:54+00:00", 'date': "2000-01-17T11:23:54+00:00",
'committer_date': "2000-01-17T11:23:54+00:00", 'committer_date': "2000-01-17T11:23:54+00:00",
'synthetic': False, 'synthetic': False,
'type': 'git', 'type': 'git',
'parents': [], 'parents': [],
'metadata': [], 'metadata': [],
'merge': False
}]) }])
mock_backend.revision_log.assert_called_with( mock_backend.revision_log.assert_called_with(
hex_to_hash('abcdbe353ed3480476f032475e7c233eff7371d5'), 100) hex_to_hash('abcdbe353ed3480476f032475e7c233eff7371d5'), 100)
@patch('swh.web.ui.service.backend') @patch('swh.web.ui.service.backend')
@istest @istest
def lookup_revision_log_by(self, mock_backend): def lookup_revision_log_by(self, mock_backend):
▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Lines def lookup_revision_log_by(self, mock_backend):
}, },
'message': 'elegant fix for bug 31415957', 'message': 'elegant fix for bug 31415957',
'date': "2000-01-17T11:23:54+00:00", 'date': "2000-01-17T11:23:54+00:00",
'committer_date': "2000-01-17T11:23:54+00:00", 'committer_date': "2000-01-17T11:23:54+00:00",
'synthetic': False, 'synthetic': False,
'type': 'git', 'type': 'git',
'parents': [], 'parents': [],
'metadata': [], 'metadata': [],
'merge': False
}]) }])
mock_backend.revision_log_by.assert_called_with( mock_backend.revision_log_by.assert_called_with(
1, 'refs/heads/master', None) 1, 'refs/heads/master', None)
@patch('swh.web.ui.service.backend') @patch('swh.web.ui.service.backend')
@istest @istest
def lookup_revision_log_by_nolog(self, mock_backend): def lookup_revision_log_by_nolog(self, mock_backend):
▲ Show 20 Lines • Show All 306 Lines • ▼ Show 20 Lines def lookup_revision_by(self, mock_backend):
# then # then
self.assertEquals(actual_revision, expected_rev) self.assertEquals(actual_revision, expected_rev)
mock_backend.revision_get_by(1, 'master2', 'some-ts') mock_backend.revision_get_by(1, 'master2', 'some-ts')
@patch('swh.web.ui.service.backend') @patch('swh.web.ui.service.backend')
@istest @istest
def lookup_revision_by_nomerge(self, mock_backend):
# given
stub_rev = {
'id': hex_to_hash('28d8be353ed3480476f032475e7c233eff7371d5'),
'directory': hex_to_hash(
'7834ef7e7c357ce2af928115c6c6a42b7e2a44e6'),
'author': {
'name': b'ynot',
'email': b'ynot@blah.org',
},
'parents': [
zackUnsubmitted
Not Done
Inline Actions

other stuff to be factored out

zack: other stuff to be factored out
hex_to_hash('adc83b19e793491b1c6ea0fd8b46cd9f32e592fc')]
}
expected_rev = {
'id': '28d8be353ed3480476f032475e7c233eff7371d5',
'directory': '7834ef7e7c357ce2af928115c6c6a42b7e2a44e6',
'author': {
'name': 'ynot',
'email': 'ynot@blah.org',
},
'parents': ['adc83b19e793491b1c6ea0fd8b46cd9f32e592fc'],
'merge': False
}
mock_backend.revision_get_by.return_value = stub_rev
# when
actual_revision = service.lookup_revision_by(10, 'master2', 'some-ts')
# then
self.assertEquals(actual_revision, expected_rev)
mock_backend.revision_get_by(1, 'master2', 'some-ts')
@patch('swh.web.ui.service.backend')
@istest
def lookup_revision_by_merge(self, mock_backend):
# given
stub_rev = {
'id': hex_to_hash('28d8be353ed3480476f032475e7c233eff7371d5'),
'directory': hex_to_hash(
'7834ef7e7c357ce2af928115c6c6a42b7e2a44e6'),
'author': {
'name': b'ynot',
'email': b'ynot@blah.org',
},
'parents': [
hex_to_hash('adc83b19e793491b1c6ea0fd8b46cd9f32e592fc'),
hex_to_hash('adc83b19e793491b1c6db0fd8b46cd9f32e592fc')
]
}
expected_rev = {
'id': '28d8be353ed3480476f032475e7c233eff7371d5',
'directory': '7834ef7e7c357ce2af928115c6c6a42b7e2a44e6',
'author': {
'name': 'ynot',
'email': 'ynot@blah.org',
},
'parents': ['adc83b19e793491b1c6ea0fd8b46cd9f32e592fc',
'adc83b19e793491b1c6db0fd8b46cd9f32e592fc'],
'merge': True
}
mock_backend.revision_get_by.return_value = stub_rev
# when
actual_revision = service.lookup_revision_by(10, 'master2', 'some-ts')
# then
self.assertEquals(actual_revision, expected_rev)
mock_backend.revision_get_by(1, 'master2', 'some-ts')
@patch('swh.web.ui.service.backend')
@istest
def lookup_revision_with_context_by_ko(self, mock_backend): def lookup_revision_with_context_by_ko(self, mock_backend):
# given # given
mock_backend.revision_get_by.return_value = None mock_backend.revision_get_by.return_value = None
# when # when
with self.assertRaises(NotFoundExc) as cm: with self.assertRaises(NotFoundExc) as cm:
origin_id = 1 origin_id = 1
branch_name = 'master3' branch_name = 'master3'
▲ Show 20 Lines • Show All 204 Lines • Show Last 20 Lines
About · Developer information · Legal