This is an archive of the discontinued LLVM Phabricator instance.

Get test/types tests passing on remote targets
ClosedPublic

Authored by vharron on Jan 27 2015, 7:31 PM.

Details

Summary

redirecting output to a path that will work well on host or target.
copying file from output location to location on local host that
test will read from

Diff Detail

Event Timeline

vharron updated this revision to Diff 18875.Jan 27 2015, 7:31 PM
vharron retitled this revision from to Get test/types tests passing on remote targets .
vharron updated this object.
vharron edited the test plan for this revision. (Show Details)
vharron added reviewers: clayborg, ovyalov.
vharron set the repository for this revision to rL LLVM.
vharron added a subscriber: Unknown Object (MLST).
ovyalov edited edge metadata.Jan 28 2015, 9:44 AM

Please see my comments.

source/Commands/CommandObjectPlatform.cpp
2284

Why we don't need mkdir.. put-file commands in non-debug configurations?

test/types/AbstractBase.py
53

inside?

91–101

You may use "platform process launch" here in order to make it work either locally or remotely.

Looks like I got my diff backwards *blush*

vharron updated this revision to Diff 18923.Jan 28 2015, 3:01 PM
vharron retitled this revision from Get test/types tests passing on remote targets to Get test/types tests passing on remote targets.
vharron updated this object.
vharron edited edge metadata.

Uses remote working directory for stdout redirect.

Better chance of working on Windows.

ovyalov added inline comments.Jan 28 2015, 3:42 PM
test/types/AbstractBase.py
94

You may use assertIsNotNone here.

94

https://github.com/llvm-mirror/lldb/blob/29b7ef5538c8f186ae9d5aa2758cebc1d1947c7e/test/lldbtest.py#L1681

It should not be a big deal but it seems lldb.remote_platform_working_dir is considered to be a main root directory and lldb.remote_platform.GetWorkingDirectory is a single test root. So, lldb.remote_platform.GetWorkingDirectory() might be a slightly more fine-grained option here.

96

s/process launch/platform process launch ?

184

Could you extract this logic into a separate method to use it here and within generic_type_tester?

zturner requested changes to this revision.Jan 28 2015, 3:53 PM
zturner added a reviewer: zturner.
zturner added a subscriber: zturner.
zturner added inline comments.
test/types/AbstractBase.py
103–105

For portability this should use the python tempfile module, and also it should not use hardcoded paths like /foo

199–202

Same as above.

This revision now requires changes to proceed.Jan 28 2015, 3:53 PM
zturner added inline comments.Jan 28 2015, 3:57 PM
test/types/AbstractBase.py
187

Can you use

os.path.join(lldb.remote_platform_working_dir, "lldb-stdout-redirect.txt")

vharron updated this revision to Diff 18932.Jan 28 2015, 5:08 PM
vharron edited edge metadata.

refactored process_launch_o into separate function
using os.path.join instead of string.format
using assertIsNotNone(

instead of assertNotEqual(None,

using lldb.remote_platform.GetWorkingDirectory()

instead of using lldb.remote_platform_working_dir
ovyalov accepted this revision.Jan 28 2015, 5:13 PM
ovyalov edited edge metadata.

Would you mind doing the temp file changes too? It wasn't your code that
did it with hardcoded paths, but since you're in there anyway, maybe it's a
quick fix?

I don't see the path you're talking about.

The one you removed then added back in the first and second patch set. It
was /tmp/golden_file.txt or something

I think because it was removed then added back. It was in the diff of diff
1 vs. diff 2. Anyway it's no big deal if it's not easy

clayborg accepted this revision.Jan 29 2015, 9:45 AM
clayborg edited edge metadata.

lgtm

Friendly ping.

vharron accepted this revision.May 9 2015, 11:33 PM
vharron added a reviewer: vharron.

Committed r228217

This revision is now accepted and ready to land.May 9 2015, 11:35 PM