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

Made the required changes

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 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.

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.

This revision now requires changes to proceed.May 6 2019, 11:42 AM

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?

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.May 10 2019, 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

Now everything works. Thanks @vlorentz.

faux marked an inline comment as done.May 14 2019, 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 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.May 14 2019, 4:31 PM

Have used unknown_revision, unknown_directory and unknown_content

faux marked an inline comment as done.May 15 2019, 7:49 PM
vlorentz added inline comments.
swh/web/tests/common/test_service.py
350–381

Code style nitpick:

revision = {
    'author': {
        'name': b'abcd',
        ...
376–385

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

This revision now requires changes to proceed.May 15 2019, 8:15 PM
faux marked 2 inline comments as done.May 15 2019, 9:20 PM

lgtm.

@anlambert Do you want to have a look?

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

You can remove that line

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

@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']}))
718–721

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
13

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
350

Yes, please remove it

351–383

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)
}
377

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

385–397

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
    }]
}
401–407

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.May 16 2019, 11:44 AM

Rebased to master made changes

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
717

Use single quotes for string literal

718

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
383–386

unindent here

388

same here

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