Page MenuHomePhabricator

normalize test file extension for test filenames
ClosedPublic

Authored by tfiala on Apr 17 2016, 9:32 PM.

Details

Reviewers
labath
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.

Diff Detail

Event Timeline

tfiala updated this revision to Diff 54031.Apr 17 2016, 9:32 PM
tfiala retitled this revision from to normalize test file extension for test filenames.
tfiala updated this object.
tfiala added a reviewer: labath.
tfiala added a subscriber: lldb-commits.
labath edited edge metadata.Apr 18 2016, 2:36 AM

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.

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.

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 updated this revision to Diff 54078.Apr 18 2016, 10:02 AM
tfiala edited edge metadata.

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

tfiala updated this revision to Diff 54079.Apr 18 2016, 10:04 AM

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

labath accepted this revision.Apr 18 2016, 10:10 AM
labath edited edge metadata.

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

This revision is now accepted and ready to land.Apr 18 2016, 10:10 AM

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.

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.

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.

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 updated this revision to Diff 54102.Apr 18 2016, 1:22 PM
tfiala edited edge metadata.

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 this revision.Apr 18 2016, 1:33 PM

Closed via commit:
r266664