Page MenuHomeSoftware Heritage

Made changes to adapt it to new content_find return type
ClosedPublic

Authored by faux on Apr 16 2019, 2:27 PM.

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Apr 16 2019, 3:26 PM
faux updated this revision to Diff 4599.Apr 16 2019, 4:37 PM

Made the required changes

vlorentz requested changes to this revision.Apr 16 2019, 5:25 PM
vlorentz added inline comments.
swh/web/common/service.py
722

This will crash if content is None.

Please fix it and add a test that checks this case.

This revision now requires changes to proceed.Apr 16 2019, 5:25 PM
faux updated this revision to Diff 4672.Sun, Apr 28, 12:46 AM
faux marked an inline comment as done.

I am getting an error when adding the test so I thought it would be better to get a review.
Also the error occurs sometimes with file unrelated to this change. About this change it shows that it is generating inconsistent data
which I am unable to diagnose.

faux updated this revision to Diff 4676.EditedMon, Apr 29, 6:29 AM

Have added the test for you to check if I am going in the correct direction.
I took these values from the terminal when I used print(revision, unknown_content)
to print the values of those.... Also have changed my implementation of "return the first element in the list otherwise none" by using the _first_element function
just to make it a bit more concise and easy to understand.

vlorentz requested changes to this revision.Mon, May 6, 11:42 AM
This revision now requires changes to proceed.Mon, May 6, 11:42 AM
faux updated this revision to Diff 4784.Fri, May 10, 9:36 PM

So basically if we want to raise notfoundexc by content_find we have to give
dir_path some value otherwise it sets entity['type'] as 'dir' by default and hence
elif condition will not be met so I have also tried doing revision_['target'] = unknown_content['sha1_git'] but the notfoundexc is never raised.
what should I do?

vlorentz requested changes to this revision.Fri, May 10, 9:51 PM

we have to give dir_path some value

so I have also tried doing revision_['target'] = unknown_content['sha1_git'] but the notfoundexc is never raised

Yes, look how test_lookup_directory_with_revision_with_path does it

This revision now requires changes to proceed.Fri, May 10, 9:51 PM

we have to give dir_path some value
so I have also tried doing revision_['target'] = unknown_content['sha1_git'] but the notfoundexc is never raised

Yes, look how test_lookup_directory_with_revision_with_path does it

Er, you already did that, and it indeed raises an exception, look at Jenkins' logs:

You can reproduce this example by temporarily adding @reproduce_failure('4.23.4', b'AXicY2TAD5iwiDESqQ5FDwAB3AAI') as a decorator on your test case
Falsifying example: test_lookup_directory_with_revision_unknown_content(self=<swh.web.tests.common.test_service.ServiceTestCase testMethod=test_lookup_directory_with_revision_unknown_content>, revision='500a697730a27eabdcdf8af4aa6137819e72f171', unknown_content={'blake2S256': '0000000000000000000000000000000000000000000000000000000000000002',
 'sha1': '0000000000000000000000000000000000000001',
 'sha1_git': '0000000000000000000000000000000000000002',
 'sha256': '0000000000000000000000000000000000000000000000000000000000000001'})
Traceback (most recent call last):
  File "/home/jenkins/workspace/DWAPPS/tox/.tox/py3/lib/python3.5/site-packages/swh/web/tests/common/test_service.py", line 359, in test_lookup_directory_with_revision_unknown_content
    cm.exception.args[0])
  File "/usr/lib/python3.5/unittest/case.py", line 1080, in assertIn
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib/python3.5/unittest/case.py", line 666, in fail
    raise self.failureException(msg)
AssertionError: 'Content not found for 500a697730a27eabdcdf8af4aa6137819e72f171' not found in "Directory or File 'README.md' pointed to by revision 500a697730a27eabdcdf8af4aa6137819e72f171 not found"

The only issue here is that your assertion does not match the error message

faux updated this revision to Diff 4801.EditedTue, May 14, 4:24 PM

Now everything works. Thanks @vlorentz.

faux marked an inline comment as done.Tue, May 14, 4:25 PM

we have to give dir_path some value
so I have also tried doing revision_['target'] = unknown_content['sha1_git'] but the notfoundexc is never raised

Yes, look how test_lookup_directory_with_revision_with_path does it

Er, you already did that, and it indeed raises an exception, look at Jenkins' logs:

You can reproduce this example by temporarily adding @reproduce_failure('4.23.4', b'AXicY2TAD5iwiDESqQ5FDwAB3AAI') as a decorator on your test case
Falsifying example: test_lookup_directory_with_revision_unknown_content(self=<swh.web.tests.common.test_service.ServiceTestCase testMethod=test_lookup_directory_with_revision_unknown_content>, revision='500a697730a27eabdcdf8af4aa6137819e72f171', unknown_content={'blake2S256': '0000000000000000000000000000000000000000000000000000000000000002',
 'sha1': '0000000000000000000000000000000000000001',
 'sha1_git': '0000000000000000000000000000000000000002',
 'sha256': '0000000000000000000000000000000000000000000000000000000000000001'})
Traceback (most recent call last):
  File "/home/jenkins/workspace/DWAPPS/tox/.tox/py3/lib/python3.5/site-packages/swh/web/tests/common/test_service.py", line 359, in test_lookup_directory_with_revision_unknown_content
    cm.exception.args[0])
  File "/usr/lib/python3.5/unittest/case.py", line 1080, in assertIn
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib/python3.5/unittest/case.py", line 666, in fail
    raise self.failureException(msg)
AssertionError: 'Content not found for 500a697730a27eabdcdf8af4aa6137819e72f171' not found in "Directory or File 'README.md' pointed to by revision 500a697730a27eabdcdf8af4aa6137819e72f171 not found"

The only issue here is that your assertion does not match the error message

vlorentz requested changes to this revision.Tue, May 14, 4:31 PM
vlorentz added inline comments.
swh/web/tests/common/test_service.py
344–396

please use unknown_revision() instead of revision(), that will spare some hacks in the test

You can also use unknown_directory() to generate a directory id, and build the directory from scratch in the test (that's easier to read)

This revision now requires changes to proceed.Tue, May 14, 4:31 PM
faux updated this revision to Diff 4840.Wed, May 15, 7:32 PM

Have used unknown_revision, unknown_directory and unknown_content

faux marked an inline comment as done.Wed, May 15, 7:49 PM
vlorentz requested changes to this revision.Wed, May 15, 8:15 PM
vlorentz added inline comments.
swh/web/tests/common/test_service.py
351–382

Code style nitpick:

revision = {
    'author': {
        'name': b'abcd',
        ...
377–386

Can you replace unknown_revision[:len(unknown_revision)-4]+"bcd1" with just unknown_revision?

This revision now requires changes to proceed.Wed, May 15, 8:15 PM
faux updated this revision to Diff 4842.Wed, May 15, 9:19 PM

Have made the changes.

faux marked 2 inline comments as done.Wed, May 15, 9:20 PM

lgtm.

@anlambert Do you want to have a look?

swh/web/tests/common/test_service.py
351

You can remove that line

@vlorentz: Sure, let me patch storage and web locally to test the changes first.

anlambert requested changes to this revision.Thu, May 16, 11:44 AM

@faux, I am fine with the updated service module implementation and the tests pass so this looks good to me.

Nevertheless, I pointed out numerous code formatting issues that you must addressed first.

Also please rebase to origin/master before updating that diff.

swh/web/common/service.py
715–720

Just a nitpick on code style here, can you change it to

content = _first_element(
    storage.content_find({'sha1_git': entity['target']}))
719–722

Same kind of formatting issue here, change it to

raise NotFoundExc('Content not found for revision %s'
                  % sha1_git)
swh/web/tests/common/test_service.py
14

As you are only using the DentryPerms enum from that module, you should change your import statement to:

from swh.model.from_disk import DentryPerms
351

Yes, please remove it

352–384

Again some nitpicks regarding code formatting, please update that dict definition to:

revision = {
    'author': {
        'name': b'abcd',
        'email': b'abcd@company.org',
        'fullname': b'abcd abcd'
    },
    'committer': {
        'email': b'aaaa@company.org',
        'fullname': b'aaaa aaa',
        'name': b'aaa'
    },
    'committer_date': {
        'negative_utc': False,
        'offset': 0,
        'timestamp': 1437511651
    },
    'date': {
        'negative_utc': False,
        'offset': 0,
        'timestamp': 1437511651
    },
    'message': b'bleh',
    'metadata': [],
    'parents': [],
    'synthetic': False,
    'type': 'file',
    'id': hash_to_bytes(unknown_revision),
    'directory': hash_to_bytes(unknown_directory)
}
378

You should use the hash_to_bytes function from swh.model.hashutil instead

386–398

Same here, update the dict definition to:

dir = {
    'id': hash_to_bytes(unknown_directory),
    'entries': [{
        'name': bytes(dir_path.encode('utf-8')),
        'type': 'file',
        'target': hash_to_bytes(unknown_content['sha1_git']),
        'perms': DentryPerms.content
    }]
}
402–408

Same code formatting issue, replace with:

with self.assertRaises(NotFoundExc) as cm:
    service.lookup_directory_with_revision(unknown_revision, dir_path)
self.assertIn('Content not found for revision %s' % unknown_revision,
              cm.exception.args[0])
This revision now requires changes to proceed.Thu, May 16, 11:44 AM
faux updated this revision to Diff 4846.Thu, May 16, 12:42 PM

Rebased to master made changes

faux marked 9 inline comments as done.Thu, May 16, 12:44 PM

Apart a couple of small fixes, that diff can be accepted as is.

Nevertheless, let's wait for the landing of D1288 before merging this.

swh/web/common/service.py
718

Use single quotes for string literal

719

the parenthesis around sha1_git are not needed when there is only a single value to interpolate in the string

swh/web/tests/common/test_service.py
384–387

unindent here

389

same here

faux updated this revision to Diff 4847.Thu, May 16, 1:38 PM

hmmmm

faux marked 4 inline comments as done.Thu, May 16, 1:51 PM
anlambert accepted this revision.Thu, May 16, 2:17 PM
vlorentz accepted this revision.Thu, May 16, 2:17 PM
This revision is now accepted and ready to land.Thu, May 16, 2:17 PM
This revision was landed with ongoing or failed builds.Thu, May 16, 2:20 PM
This revision was automatically updated to reflect the committed changes.
faux added a comment.Thu, May 16, 2:21 PM

hehehehe.