[Lldb-commits] [PATCH] D58748: [ExpressionParser] Test GetClangResourceDir

Thu Feb 28 12:06:57 PST 2019

xiaobai marked an inline comment as done.
xiaobai added inline comments.

================
Comment at: packages/Python/lldbsuite/test/functionalities/paths/TestPaths.py:20
+    # GetClangResourceDir doesn't work on windows yet
+    @expectedFailureAll(oslist=["windows"])
     @no_debug_info_test
----------------
labath wrote:
> xiaobai wrote:
> > teemperor wrote:
> > > Doesn't that mean that we no longer test any of the paths on windows? E.g. if someone breaks `lldb.ePathTypeLLDBShlibDir` we will not see this test failing on Windows. I would prefer of extracting the ClangDir test into it's own method that we mark as failing.
> > Technically it is being tested but it is expected to fail. That being said, I don't mind extracting the ClangDir test into its own method while I work on implementing it, but I don't think it needs its own test. Maybe after I implement `GetClangResourceDir` on windows I can merge the tests? :P 
> That sounds fine, though I have to say that these tests don't really test much. There is a test for the MacOS functionality here in unittests/Expression/ClangParserTest.cpp, which is much more detailed. It would be great if you could add a windows version there too.
I originally added this test there, but testing things there as-is is presents a disadvantage: The unittests don't link against liblldb but the individual lldb libraries, so the methods that figure out where the Clang Resource Dir (and pretty much any other directory for that matter) will report something like this:
`/path/to/llvm/build/tools/lldb/unittests/lib/clang/9.0.0`. So, the best this test could do is set that the filespec had a directory and not a filename set, which is what this test already does.

That being said, the way MacOS is being tested is much nicer. I could do a small refactor so the non-MacOS posix implementation is closer to the MacOS implementation and add a Linux test (and eventually a windows test) to `unittests/Expression/ClangParserTest.cpp` as well. What do you think?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58748/new/

https://reviews.llvm.org/D58748