normalize test file extension for test filenames

[tfiala]

Summary

Ensure all uses of a test filename in the test event infrastructure normalize the test filename to end in the ".py" extension. Something here changed recently such that normal code paths through a test run are producing ".pyc" instead of ".py". This caused places that need to generate the filename to differ - e.g. when a timeout or exceptional exit occurs, because those are using the straight-up .py file. This, ultimately, led to test infrastructure having multiple results for the same test when a test timed out or had an exceptional exit. This, in turn, caused a test run to fail when a rerun would normally have cleared the failure.

This wreaked havoc on the Swift CI after I took a recent merge from LLVM.org. This change is now up there and, combined with D19214, addressed all the book-keeping issues we were hitting. I'm now bringing these back to the LLDB svn repo.

The change itself is simple and just ensures all uses of the test filename change any names ending in .pyc to .py.

[labath]

So, I think I have already fixed the problem with r266192.

This seems a bit like using a sledgehammer to kill a fly. If anything, I'd add an assert somewhere that makes sure the file names are in the right form to begin with.

[tfiala]

In D19215#403844, @labath wrote:

So, I think I have already fixed the problem with r266192.

I was able to replicate just prior to applying the change, but I'll double check.

Regarding r266192 (which came through when I wasn't looking, sorry), that started showing up because somebody changed something where the test_filename attribute was no longer showing up on test case instances every place where it used to show up. I suspect there was some test refactoring that did that. (So it was a latent bug, on a code path that didn't used to get hit).

This seems a bit like using a sledgehammer to kill a fly. If anything, I'd add an assert somewhere that makes sure the file names are in the right form to begin with.

[tfiala]

I was able to replicate just prior to applying the change, but I'll double check.

Yeah that change didn't cover all the cases. The test_filename that comes from this call stack (in startTest), originating within the unittest2 framework, has the .pyc (assuming this patch here, modifying the call to inspect.getfile() to inspect.getsource()):

Traceback (most recent call last):
  File "test/dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/dotest.py", line 1083, in run_suite
    resultclass=test_result.LLDBTestResult).run(configuration.suite)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/runner.py", line 162, in run
    test(result)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 65, in __call__
    return self.run(*args, **kwds)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 85, in run
    self._wrapped_run(result)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 115, in _wrapped_run
    test._wrapped_run(result, debug)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/suite.py", line 117, in _wrapped_run
    test(result)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/case.py", line 433, in __call__
    return self.run(*args, **kwds)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/case.py", line 338, in run
    result.startTest(self)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/test_result.py", line 133, in startTest
    EventBuilder.event_for_start(test))
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/result_formatter.py", line 366, in event_for_start
    test, EventBuilder.TYPE_TEST_START)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/result_formatter.py", line 269, in _event_dictionary_common
    test_filename = EventBuilder._normalize_test_filename(test.test_filename)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/result_formatter.py", line 249, in _normalize_test_filename
    raise Exception("filename ends in .pyc: {}".format(test_filename))
Exception: filename ends in .pyc: /Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/lang/cpp/lambdas/TestLambdas.pyc

So we still want this I think. I also disagree about it being a sledgehammer - it is ensuring that input parameters that come from several different sources all follow the protocol of being .py files. That's just verifying input parameters. If they all came from the same place, I'd consider it an error to correct, but when they come from places like unittest2 that we don't need/want to change, this seems like fair game.

[tfiala]

Change inspect.getfile() => inspect.getsourcefile().

[tfiala]

Whoops - don't include the 'raise' call.

[labath]

Ok, makes sense then, thanks for making sure it's necessary.

I don't quite understand how the string makes it's way there though, as the only place setting this field in the entire repository seems to be lldbinline.py

[zturner]

It seems very strange to me to be changing a .pyc filename to a .py
filename. I think we should try to understand where the .pyc filename is
coming from to begin with, and this is just masking the real error.

[tfiala]

In D19215#404143, @zturner wrote:

It seems very strange to me to be changing a .pyc filename to a .py
filename. I think we should try to understand where the .pyc filename is
coming from to begin with, and this is just masking the real error.

Testing: 38 test suites, 12 threads
7 out of 38 test suites processed - TestChar1632T.py

Traceback (most recent call last):
  File "test/dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/dotest.py", line 952, in run_suite
    visit('Test', dirpath, filenames)
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/dotest.py", line 747, in visit
    configuration.suite.addTests(unittest2.defaultTestLoader.loadTestsFromName(base))
  File "/Volumes/Data/src/lldb-llvm.org/lldb/third_party/Python/module/unittest2/unittest2/loader.py", line 103, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/lang/cpp/lambdas/TestLambdas.py", line 4, in <module>
    lldbinline.MakeInlineTest(__file__, globals(), [])
  File "/Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/lldbinline.py", line 210, in MakeInlineTest
    raise Exception("lldbinline file ends with .pyc: {}".format(__file))
Exception: lldbinline file ends with .pyc: /Volumes/Data/src/lldb-llvm.org/lldb/packages/Python/lldbsuite/test/lang/cpp/lambdas/TestLambdas.pyc

The file for lldbinline.py-based tests is getting set to .pyc via the unittest2 loader. This callstack comes from inserting this code into lldbinline.py:

diff --git a/packages/Python/lldbsuite/test/lldbinline.py b/packages/Python/lldbsuite/test/lldbinline.py
index 4eaa2a7..3eef4ee 100644
--- a/packages/Python/lldbsuite/test/lldbinline.py
+++ b/packages/Python/lldbsuite/test/lldbinline.py
@@ -206,6 +206,10 @@ def MakeInlineTest(__file, __globals, decorators=None):
     __globals.update({test_name : test})

     # Store the name of the originating file.o
-    test.test_filename = __file
+    if __file is not None and __file.endswith(".pyc"):
+        raise Exception("lldbinline file ends with .pyc: {}".format(__file))
+        test.test_filename = __file[0:-1]
+    else:
+        test.test_filename = __file
     return test

So - I can either intercept it right there and convert to .py, or I can protect it at the API ingestion level. If we go with the former, then I want to add an assert to the latter to make sure any cases of this sneaking in are caught.

Since this case was only happening for lldbinline tests, I suspect it was always happening and just wasn't caught.

I'll adjust the patch shortly to have the API ingestion of the test_filename assert on .pyc (and I'll ensure it ends in .py, not just that it *isn't* .pyc). I'll also fix up the lldbinline case at the point where we grab it from the unittest2 loader code.

[tfiala]

The file for lldbinline.py-based tests is getting set to .pyc via the unittest2 loader. This callstack comes from inserting this code into lldbinline.py:

diff --git a/packages/Python/lldbsuite/test/lldbinline.py b/packages/Python/lldbsuite/test/lldbinline.py
index 4eaa2a7..3eef4ee 100644
--- a/packages/Python/lldbsuite/test/lldbinline.py
+++ b/packages/Python/lldbsuite/test/lldbinline.py
@@ -206,6 +206,10 @@ def MakeInlineTest(__file, __globals, decorators=None):
     __globals.update({test_name : test})

     # Store the name of the originating file.o
-    test.test_filename = __file
+    if __file is not None and __file.endswith(".pyc"):
+        raise Exception("lldbinline file ends with .pyc: {}".format(__file))
+        test.test_filename = __file[0:-1]
+    else:
+        test.test_filename = __file
     return test

So - I can either intercept it right there and convert to .py, or I can protect it at the API ingestion level. If we go with the former, then I want to add an assert to the latter to make sure any cases of this sneaking in are caught.

I should mention that I verified we do *not* instruct unittest2 to load a .pyc (i.e. this isn't happening because we somehow asked it to parse a .pyc):

diff --git a/packages/Python/lldbsuite/test/dotest.py b/packages/Python/lldbsuite/test/dotest.py
index 198e87c..71d8c58 100644
--- a/packages/Python/lldbsuite/test/dotest.py
+++ b/packages/Python/lldbsuite/test/dotest.py
@@ -744,6 +744,8 @@ def visit(prefix, dir, names):
                 # A simple case of just the module name.  Also the failover case
                 # from the filterspec branch when the (base, filterspec) combo
                 # doesn't make sense.
+                if base is not None and base.endswith(".pyc"):
+                   raise Exception("base ends with .pyc: {}".format(base))
                 configuration.suite.addTests(unittest2.defaultTestLoader.loadTestsFromName(base))

I'm not hitting that when I hit the callstack listed above.

[tfiala]

Side note:

Okay I think the mystery of this was caught by Jim. When we started accepting usage of .pycs again, we're allowing the python runtime to select using the pyc if available and not older than the .py file. This makes it possible to get a test file that is sometimes the .py and sometimes the .pyc.

I'm going to be looking into getting a verification mechanism for the test infrastructure that covers items like this. This caused a whole lot of trouble on CI and, had we had tests for the test infrastructure itself, we could have avoided this. I'll take an action item on that.

[tfiala]

Final change: this change:

1. has the lldbinline.py test intercept its __file parameter and convert from .pyc to .py before storing to the test.test_filename attribute.
2. has the test event creation mechanism and test_event usage mechanism always validate that the filename ends in .py. It will generate an exception if not.

If we find that #2 is triggering sometimes, I want to go back to being permissive in input now that it is clear this started when we removed the disabling feature that prohibited .pyc files from being generated.

[tfiala]

Closed via commit:
r266664

packages/Python/lldbsuite/test/lldbinline.py

     
 
 def MakeInlineTest(__file, __globals, decorators=None):
+    # Adjust the filename if it ends in .pyc.  We want filenames to
+    # reflect the source python file, not the compiled variant.
+    if __file is not None and __file.endswith(".pyc"):
+        # Strip the trailing "c"
+        __file = __file[0:-1]
+
     # Derive the test name from the current file name
     file_basename = os.path.basename(__file)
     InlineTest.mydir = TestBase.compute_mydir(__file)

     # Add the test case to the globals, and hide InlineTest
     __globals.update({test_name : test})
 
-    # Store the name of the originating file.o
+    # Keep track of the original test filename so we report it
+    # correctly in test results.
     test.test_filename = __file
     return test
 

packages/Python/lldbsuite/test/result_formatter.py

     def create_socket(port):
         """Creates a socket to the localhost on the given port.
 
-        @param port the port number of the listenering port on
+        @param port the port number of the listening port on
         the localhost.
 
         @return (socket object, socket closing function)

         return event
 
     @staticmethod
+    def _assert_is_python_sourcefile(test_filename):
+        if test_filename is not None:
+            if not test_filename.endswith(".py"):
+                raise Exception("source python filename has unexpected extension: {}".format(test_filename))
+        return test_filename
+
+    @staticmethod
     def _event_dictionary_common(test, event_type):
         """Returns an event dictionary setup with values for the given event type.
 

         # Determine the filename for the test case.  If there is an attribute
         # for it, use it.  Otherwise, determine from the TestCase class path.
         if hasattr(test, "test_filename"):
-            test_filename = test.test_filename
+            test_filename = EventBuilder._assert_is_python_sourcefile(test.test_filename)
         else:
-            test_filename = inspect.getsourcefile(test.__class__)
+            test_filename = EventBuilder._assert_is_python_sourcefile(inspect.getsourcefile(test.__class__))
 
         event = EventBuilder.bare_event(event_type)
         event.update({

         if exception_description is not None:
             event["exception_description"] = exception_description
         if test_filename is not None:
-            event["test_filename"] = test_filename
+            event["test_filename"] = EventBuilder._assert_is_python_sourcefile(test_filename)
         if command_line is not None:
             event["command_line"] = command_line
         return event

         if worker_index is not None:
             event["worker_index"] = int(worker_index)
         if test_filename is not None:
-            event["test_filename"] = test_filename
+            event["test_filename"] = EventBuilder._assert_is_python_sourcefile(test_filename)
         if command_line is not None:
             event["command_line"] = command_line
         return event