[Lldb-commits] [PATCH] D42763: Build each testcase variant in its own subdirectory and remove the srcdir lock file

[Alex Langford via Phabricator via lldb-commits]

xiaobai added inline comments.


================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/comp_dir_symlink/TestCompDirSymLink.py:39-40
             (_COMP_DIR_SYM_LINK_PROP, pwd_symlink))
-        lldbutil.run_break_set_by_file_and_line(self, self.src_path, self.line)
+        src_path = self.getBuildArtifact(_SRC_FILE)
+        lldbutil.run_break_set_by_file_and_line(self, src_path, self.line)
 
----------------
Instead of making a temporary variable `src_path` that gets used once in each instance, why not just insert `self.getBuildArtifact(_SRC_FILE)` directly into `lldbutil.run_break_set_by_file_and_line()`?


================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_on_vectors/TestValueOfVectorVariable.py:25-26
         """Test verify displayed value of vector variable."""
-        self.build(dictionary=self.d)
-        self.setTearDownCleanup(dictionary=self.d)
+        exe = self.getBuildArtifact("a.out")
+        d = {'C_SOURCES': self.source, 'EXE': exe}
+        self.build(dictionary=d)
----------------
You should be able to insert the call to `getBuildArtifact()` into the construction of `d`, since `exe` is used nowhere else.


================
Comment at: packages/Python/lldbsuite/test/functionalities/watchpoint/watchpoint_on_vectors/TestValueOfVectorVariable.py:40
         exe = self.getBuildArtifact("a.out")
+        d = {'C_SOURCES': self.source, 'EXE': exe}
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
----------------
Is this `d` used?


================
Comment at: packages/Python/lldbsuite/test/lldbinline.py:96-98
+            return "-N dwarf %s" % (testdir)
         else:
+            return "-N dsym %s" % (testdir)
----------------
Good opportunity to move away from the old style of formatting strings to a newer style? It doesn't make a huge difference, I just think it'd be nice to do. :)
`return "-N dwarf {}".format(testdir)`
`return "-N dwarf {}".format(testdir)`

This is supported in Python 2.7, but I'm not sure if we can assume that version or greater to be present.


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:533-539
+        if rel_prefix[-3:] == ".py":
+            rel_prefix = rel_prefix[:-3]
+        elif rel_prefix[-4:] == ".pyc":
+            rel_prefix = rel_prefix[:-4]
+        else:
+            raise Exception("test_file is not a python file")
+        return os.path.split(rel_prefix)
----------------
Might I suggest this?
```
(rel_test_path, extension) = os.path.splitext(rel_prefix)
if extension not in [".py", ".pyc"]:
    raise Exception("test_file is not a python file")
return rel_test_path
```
Feels cleaner to me, at least. :)


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:712
         """Return the full path to the current test."""
-        return os.path.join(os.environ["LLDB_TEST"], self.mydir)
+        subdir, stem = self.mydir
+        return os.path.join(os.environ["LLDB_TEST"], subdir)
----------------
`stem` is unused, you could make this a `_`.


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:718
+        variant = self.debug_info
+        subdir, stem = self.mydir
+        if not variant:
----------------
`subdir` and `stem` are kind of confusing to me. It looks like you used `test_subdir` below, so it might be good to use that here too, I think. Also, I think the name `test_name` is less confusing than `test_stem`. What do you think?


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:719
+        subdir, stem = self.mydir
+        if not variant:
+            variant = 'default'
----------------
I recommend `if variant is None` instead of `if not variant`


================
Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:63
     # Construct the base make invocation.
+    test_subdir, test_stem = dir_stem
     lldb_test = os.environ["LLDB_TEST"]
----------------
I find `test_stem` kind of confusing. How do you feel about `test_name`?


https://reviews.llvm.org/D42763