Page MenuHomePhabricator

Fix TestQuoting on remote targets.
ClosedPublic

Authored by chaoren on Mar 2 2015, 6:00 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

chaoren updated this revision to Diff 21070.Mar 2 2015, 6:00 PM
chaoren retitled this revision from to Fix TestQuoting on remote targets..
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added reviewers: ovyalov, clayborg.
chaoren added a subscriber: Unknown Object (MLST).
ovyalov edited edge metadata.Mar 2 2015, 6:14 PM

Please see my comments.

test/settings/quoting/TestQuoting.py
71 ↗(On Diff #21070)

No need for semicolon

74 ↗(On Diff #21070)

Does it work in case of local run? It seems it might read and write data from/into the same file..

chaoren added inline comments.Mar 2 2015, 7:24 PM
test/settings/quoting/TestQuoting.py
74 ↗(On Diff #21070)

I had that concern too, but it works fine on local.

ovyalov added inline comments.Mar 2 2015, 8:36 PM
test/settings/quoting/TestQuoting.py
74 ↗(On Diff #21070)

I remember I had such issue with platform.Put when source and destination were in the same directory.
To be safe, please give them different names stdout-(local|remote).txt and check on OSX as well.

chaoren added inline comments.Mar 2 2015, 8:56 PM
test/settings/quoting/TestQuoting.py
74 ↗(On Diff #21070)

I guess we could just check if the file already exists, and if it does, assume local, and skip the Get(). Platform cannot tell if we're local, right? Also, platform doesn't provide any functionality to delete remote files, right?

chaoren updated this revision to Diff 21079.Mar 2 2015, 9:01 PM
chaoren edited edge metadata.

Disable on local.

tberghammer added inline comments.
test/settings/quoting/TestQuoting.py
71 ↗(On Diff #21079)

I think you can check if we run against a remote platform with this condition:

if lldb.remote_platform:
    # Remote
else:
    # Local
74 ↗(On Diff #21070)

Platform can remove a file (with the Unlink command) but it is not exposed to the Python interface. If you need it you can expose it easily.

clayborg requested changes to this revision.Mar 3 2015, 9:42 AM
clayborg edited edge metadata.

Please use the "if lldb.remote_platform:" test when doing any remote fetching as tberghammer suggested.

This revision now requires changes to proceed.Mar 3 2015, 9:42 AM
chaoren edited edge metadata.

Using lldb.remote_platform to check if file transfer is necessary.

clayborg requested changes to this revision.Mar 3 2015, 10:41 AM
clayborg edited edge metadata.

Check my inline comments.

test/settings/quoting/TestQuoting.py
72 ↗(On Diff #21118)

Shouldn't this be set to "False" to not resolve the remote copy? Isn't "src_file_spec" the remote path?

73 ↗(On Diff #21118)

Isn't this the local copy of the file and so it should be resolved?

This revision now requires changes to proceed.Mar 3 2015, 10:41 AM
chaoren edited edge metadata.

Accidentally resolved remote file instead of local.

clayborg accepted this revision.Mar 3 2015, 10:58 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Mar 3 2015, 10:58 AM
This revision was automatically updated to reflect the committed changes.
vharron added a subscriber: vharron.Mar 4 2015, 7:00 PM
vharron added inline comments.
lldb/trunk/test/settings/quoting/TestQuoting.py
71

In other tests, I use the terms "local" and "remote" instead of "src" and "dst"