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
Lint Skipped
Unit
Unit Tests Skipped

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 4845

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