Page MenuHomeSoftware Heritage
Differential D1420

Made changes to adapt it to new content_find return type
ClosedPublic
Actions

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

Details

Reviewers
vlorentz
anlambert
Group Reviewers
Reviewers
Maniphest Tasks
T1349: Storage.content_find should return all matches, not just one.
Commits
rDWAPPSed61da4501cf: Made changes to adapt it to new content_find return type and added the test for…
Summary

Made changes in files using content_find method

Depends on D1288

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

swh-public-ci added a comment.Apr 16 2019, 4:41 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/390/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/390/console

Harbormaster failed remote builds in B5464: Diff 4599!Apr 16 2019, 4:41 PM
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.Apr 28 2019, 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.

swh-public-ci added a comment.Apr 28 2019, 12:50 AM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/409/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/409/console

Harbormaster failed remote builds in B5553: Diff 4672!Apr 28 2019, 12:50 AM
faux updated this revision to Diff 4676.EditedApr 29 2019, 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.

swh-public-ci added a comment.Apr 29 2019, 6:32 AM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/410/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/410/console

Harbormaster failed remote builds in B5556: Diff 4676!Apr 29 2019, 6:33 AM
anlambert edited the summary of this revision. (Show Details)May 2 2019, 11:33 AM
anlambert added a parent revision: D1288: Storage.content_find returns list instead of single value.
vlorentz requested changes to this revision.May 6 2019, 11:42 AM
This revision now requires changes to proceed.May 6 2019, 11:42 AM
faux updated this revision to Diff 4784.May 10 2019, 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?

swh-public-ci added a comment.May 10 2019, 9:41 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/435/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/435/console

Harbormaster failed remote builds in B5684: Diff 4784!May 10 2019, 9:41 PM
vlorentz requested changes to this revision.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

This revision now requires changes to proceed.May 10 2019, 9:51 PM
vlorentz added a comment.May 10 2019, 9:54 PM
In D1420#32407, @vlorentz wrote:

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.EditedMay 14 2019, 4:24 PM

Now everything works. Thanks @vlorentz.

faux marked an inline comment as done.May 14 2019, 4:25 PM
In D1420#32410, @vlorentz wrote:
In D1420#32407, @vlorentz wrote:

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

swh-public-ci added a comment.May 14 2019, 4:29 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/436/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/436/console

Harbormaster failed remote builds in B5701: Diff 4801!May 14 2019, 4:29 PM
vlorentz requested changes to this revision.May 14 2019, 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.May 14 2019, 4:31 PM
faux updated this revision to Diff 4840.May 15 2019, 7:32 PM

Have used unknown_revision, unknown_directory and unknown_content

swh-public-ci added a comment.May 15 2019, 7:38 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/437/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/437/console

Harbormaster failed remote builds in B5744: Diff 4840!May 15 2019, 7:38 PM
faux marked an inline comment as done.May 15 2019, 7:49 PM
vlorentz requested changes to this revision.May 15 2019, 8:15 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 updated this revision to Diff 4842.May 15 2019, 9:19 PM

Have made the changes.

faux marked 2 inline comments as done.May 15 2019, 9:20 PM
swh-public-ci added a comment.May 15 2019, 9:24 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/439/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/439/console

Harbormaster failed remote builds in B5745: Diff 4842!May 15 2019, 9:24 PM
vlorentz added a subscriber: anlambert.May 16 2019, 10:52 AM

lgtm.

@anlambert Do you want to have a look?

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

You can remove that line

anlambert added a comment.May 16 2019, 10:56 AM

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

anlambert requested changes to this revision.May 16 2019, 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']}))
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
faux updated this revision to Diff 4846.May 16 2019, 12:42 PM

Rebased to master made changes

faux marked 9 inline comments as done.May 16 2019, 12:44 PM
swh-public-ci added a comment.May 16 2019, 12:47 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/440/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/440/console

Harbormaster failed remote builds in B5749: Diff 4846!May 16 2019, 12:47 PM
anlambert added a comment.May 16 2019, 1:26 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
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 updated this revision to Diff 4847.May 16 2019, 1:38 PM

hmmmm

swh-public-ci added a comment.May 16 2019, 1:43 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/441/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/441/console

Harbormaster failed remote builds in B5750: Diff 4847!May 16 2019, 1:43 PM
faux marked 4 inline comments as done.May 16 2019, 1:51 PM
swh-public-ci added a comment.May 16 2019, 2:15 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/442/ for more details.

anlambert accepted this revision.May 16 2019, 2:17 PM
vlorentz accepted this revision.May 16 2019, 2:17 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
Closed by commit rDWAPPSed61da4501cf: Made changes to adapt it to new content_find return type and added the test for… (authored by faux). · Explain Why
This revision was automatically updated to reflect the committed changes.
swh-public-ci added a comment.May 16 2019, 2:20 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/443/ for more details.

Harbormaster completed remote builds in B5750: Diff 4847.May 16 2019, 2:20 PM
faux added a comment.May 16 2019, 2:21 PM

hehehehe.

Revision Contents
Changeset List

PathSize
swh/
web/
common/
19 lines
tests/
common/
60 lines

Diff 4849

View Options

swh/web/common/service.py

Loading...
View Options

swh/web/tests/common/test_service.py

Loading...
About · Developer information · Legal