This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1849

lfs: remove internal url in test
ClosedPublic

Authored by quark on Jan 11 2018, 12:27 AM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Commits
rHG2c6ebd0c850e: lfs: remove internal url in test
rHG62ec61e45673: lfs: remove internal url in test
Summary

test-lfs-test-server.t refers to a FB internal domain and requires certain
implementation (ex. set error code to 404) at that endpoint. Without any
workaround, It should in theory error out like "Domain cannot be resolved".
I don't know how Matt Harbison ran the test.

This patch changes the test to only depend on lfs-test-server.
Unfortunately the logic has to be changed since lfs-test-server does not
set error code to 404 but just removes "download" from "actions".

Diff Detail

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

Event Timeline

quark created this revision.Jan 11 2018, 12:27 AM
mharbison72 added a subscriber: mharbison72.Jan 11 2018, 1:04 AM

Yeah, I’ve been ignoring this failure. I thought I mentioned it back when this was first brought over. I’m assuming that FB will start using the built in extension and tests soon, and I didn’t want to drop test coverage on you like that. Thanks for fixing it.

hgext/lfs/blobstore.py
239

It looks like we still need the ‘if p’ test.

quark updated this revision to Diff 4845.Jan 16 2018, 1:37 PM
quark added inline comments.Jan 16 2018, 1:38 PM
hgext/lfs/blobstore.py
239

Sorry. I missed the comment. Fixed now.

mharbison72 added a comment.Jan 16 2018, 10:01 PM

LGTM, thanks.

Any thoughts on what we should do if 'p' does come up None? It's a clear server error (sending back an oid we didn't ask for).

yuja accepted this revision.Jan 17 2018, 9:30 AM
This revision is now accepted and ready to land.Jan 17 2018, 9:30 AM
Closed by commit rHG62ec61e45673: lfs: remove internal url in test (authored by quark). · Explain WhyJan 17 2018, 9:47 AM
This revision was automatically updated to reflect the committed changes.
quark added a comment.Jan 17 2018, 6:00 PM
In D1849#31622, @mharbison72 wrote:

LGTM, thanks.
Any thoughts on what we should do if 'p' does come up None? It's a clear server error (sending back an oid we didn't ask for).

Not sure. Blame the server somehow?

Revision Contents
Changeset List

PathPackages
Mhgext/lfs/blobstore.py (27 lines)
Mtests/test-lfs-test-server.t (21 lines)

Diff 4851

hgext/lfs/blobstore.py

% (ex, action)) % (ex, action))
try: try:
response = json.loads(rawjson) response = json.loads(rawjson)
except ValueError: except ValueError:
raise LfsRemoteError(_('LFS server returns invalid JSON: %s') raise LfsRemoteError(_('LFS server returns invalid JSON: %s')
% rawjson) % rawjson)
return response return response
def _checkforservererror(self, pointers, responses): def _checkforservererror(self, pointers, responses, action):
"""Scans errors from objects """Scans errors from objects
Returns LfsRemoteError if any objects has an error""" Returns LfsRemoteError if any objects has an error"""
for response in responses: for response in responses:
error = response.get('error') # The server should return 404 when objects cannot be found. Some
if error: # server implementation (ex. lfs-test-server) does not set "error"
# but just removes "download" from "actions". Treat that case
# as the same as 404 error.
notfound = (response.get('error', {}).get('code') == 404
or (action == 'download'
and action not in response.get('actions', [])))
if notfound:
ptrmap = {p.oid(): p for p in pointers} ptrmap = {p.oid(): p for p in pointers}
p = ptrmap.get(response['oid'], None) p = ptrmap.get(response['oid'], None)
if error['code'] == 404 and p: if p:
mharbison72Unsubmitted
Not Done

It looks like we still need the ‘if p’ test.

mharbison72: It looks like we still need the ‘if p’ test.
quarkAuthorUnsubmitted
Not Done

Sorry. I missed the comment. Fixed now.

quark: Sorry. I missed the comment. Fixed now.
filename = getattr(p, 'filename', 'unknown') filename = getattr(p, 'filename', 'unknown')
raise LfsRemoteError( raise LfsRemoteError(
_(('LFS server error. Remote object ' _(('LFS server error. Remote object '
'for "%s" not found: %r')) % (filename, response)) 'for "%s" not found: %r')) % (filename, response))
if 'error' in response:
raise LfsRemoteError(_('LFS server error: %r') % response) raise LfsRemoteError(_('LFS server error: %r') % response)
def _extractobjects(self, response, pointers, action): def _extractobjects(self, response, pointers, action):
"""extract objects from response of the batch API """extract objects from response of the batch API
response: parsed JSON object returned by batch API response: parsed JSON object returned by batch API
return response['objects'] filtered by action return response['objects'] filtered by action
raise if any object has an error raise if any object has an error
""" """
# Scan errors from objects - fail early # Scan errors from objects - fail early
objects = response.get('objects', []) objects = response.get('objects', [])
self._checkforservererror(pointers, objects) self._checkforservererror(pointers, objects, action)
# Filter objects with given action. Practically, this skips uploading # Filter objects with given action. Practically, this skips uploading
# objects which exist in the server. # objects which exist in the server.
filteredobjects = [o for o in objects if action in o.get('actions', [])] filteredobjects = [o for o in objects if action in o.get('actions', [])]
# But for downloading, we want all objects. Therefore missing objects
# should be considered an error.
if action == 'download':
if len(filteredobjects) < len(objects):
missing = [o.get('oid', '?')
for o in objects
if action not in o.get('actions', [])]
raise LfsRemoteError(
_('LFS server claims required objects do not exist:\n%s')
% '\n'.join(missing))
return filteredobjects return filteredobjects
def _basictransfer(self, obj, action, localstore): def _basictransfer(self, obj, action, localstore):
"""Download or upload a single object using basic transfer protocol """Download or upload a single object using basic transfer protocol
obj: dict, an object description returned by batch API obj: dict, an object description returned by batch API
action: string, one of ['upload', 'download'] action: string, one of ['upload', 'download']

tests/test-lfs-test-server.t

$ echo FFFFF > b $ echo FFFFF > b
$ hg commit -m b -A b $ hg commit -m b -A b
$ echo FFFFF >> b $ echo FFFFF >> b
$ hg commit -m b b $ hg commit -m b b
$ rm -rf .hg/store/lfs $ rm -rf .hg/store/lfs
$ rm -rf `hg config lfs.usercache` $ rm -rf `hg config lfs.usercache`
$ hg update -C '.^' $ hg update -C '.^'
abort: LFS server claims required objects do not exist: abort: LFS server error. Remote object for "b" not found:(.*)! (re)
8e6ea5f6c066b44a0efa43bcce86aea73f17e6e23f0663df0251e7524e140a13!
[255] [255]
Check error message when object does not exist: Check error message when object does not exist:
$ cd $TESTTMP
$ hg init test && cd test $ hg init test && cd test
$ echo "[extensions]" >> .hg/hgrc $ echo "[extensions]" >> .hg/hgrc
$ echo "lfs=" >> .hg/hgrc $ echo "lfs=" >> .hg/hgrc
$ echo "[lfs]" >> .hg/hgrc $ echo "[lfs]" >> .hg/hgrc
$ echo "threshold=1" >> .hg/hgrc $ echo "threshold=1" >> .hg/hgrc
$ echo a > a $ echo a > a
$ hg add a $ hg add a
$ hg commit -m 'test' $ hg commit -m 'test'
$ echo aaaaa > a $ echo aaaaa > a
$ hg commit -m 'largefile' $ hg commit -m 'largefile'
$ hg debugdata .hg/store/data/a.i 1 # verify this is no the file content but includes "oid", the LFS "pointer". $ hg debugdata .hg/store/data/a.i 1 # verify this is no the file content but includes "oid", the LFS "pointer".
version https://git-lfs.github.com/spec/v1 version https://git-lfs.github.com/spec/v1
oid sha256:bdc26931acfb734b142a8d675f205becf27560dc461f501822de13274fe6fc8a oid sha256:bdc26931acfb734b142a8d675f205becf27560dc461f501822de13274fe6fc8a
size 6 size 6
x-is-binary 0 x-is-binary 0
$ cd .. $ cd ..
$ rm -rf `hg config lfs.usercache` $ rm -rf `hg config lfs.usercache`
$ hg --config 'lfs.url=https://dewey-lfs.vip.facebook.com/lfs' clone test test2
(Restart the server in a different location so it no longer has the content)
$ $PYTHON $RUNTESTDIR/killdaemons.py $DAEMON_PIDS
$ rm $DAEMON_PIDS
$ mkdir $TESTTMP/lfs-server2
$ cd $TESTTMP/lfs-server2
#if no-windows
$ lfs-test-server &> lfs-server.log &
$ echo $! >> $DAEMON_PIDS
#else
$ $PYTHON $TESTTMP/spawn.py >> $DAEMON_PIDS
#endif
$ cd $TESTTMP
$ hg clone test test2
updating to branch default updating to branch default
abort: LFS server error. Remote object for "a" not found:(.*)! (re) abort: LFS server error. Remote object for "a" not found:(.*)! (re)
[255] [255]
$ $PYTHON $RUNTESTDIR/killdaemons.py $DAEMON_PIDS $ $PYTHON $RUNTESTDIR/killdaemons.py $DAEMON_PIDS