Page MenuHomePhabricator

Get rid of global variables in dotest.py
ClosedPublic

Authored by zturner on Dec 7 2015, 4:38 PM.

Details

Reviewers
clayborg
tfiala
Summary

Since this is a big patch, you may just want to apply it locally and verify that nothing breaks. I've tested both the old and new result formatter paths, and everything seems fine. But unfortunately with Python you need 100% code coverage to be sure. So let me know if this breaks anything.

Get rid of global variables in dotest.py

This moves all the global variables into a separate module called
`configuration`.  This has a number of advantages:

1. Configuration data is centrally maintained so it's easy to get
   a high level overview of what configuration data the test suite
   makes use of.
2. The method of sharing configuration data among different parts
   of the test suite becomes standardized.  Previously we would
   put some things into the `lldb` module, some things into the
   `lldbtest_config` module, and some things would not get shared.
   Now everything is shared through one module and is available to
   the entire test suite.
3. It opens the door to moving some of the initialization code into
   the `configuration` module, simplifying the implementation of
   `dotest.py`.

There are a few stragglers that didn't get converted over to using
the `configuration` module in this patch, because it would have grown
the size of the patch unnecessarily.  This includes everything
currently in the `lldbtest_config` module, as well as the
`lldb.remote_platform` variable.  We can address these in the future.

Diff Detail

Event Timeline

zturner updated this revision to Diff 42127.Dec 7 2015, 4:38 PM
zturner retitled this revision from to Get rid of global variables in dotest.py.
zturner updated this object.
zturner added reviewers: tfiala, clayborg.
zturner added a subscriber: lldb-commits.

This is mostly mechanical, and since it looks like we have 2 people making big changes at the same time I'll go ahead and put this in so we don't step on each others' toes. But feel free to revert if this breaks anything. If you do experience any errors, it is most likely to be an unbound variable reference, which you can probably fix by changing x to configuration.x. But I think I got everything.

tfiala accepted this revision.Dec 7 2015, 9:26 PM
tfiala edited edge metadata.

Yep, sounds good. I saw it go in. I'll do a couple runs here but I'm sure we'll fix up anything if something breaks. I much prefer to get it in :-)

This revision is now accepted and ready to land.Dec 7 2015, 9:26 PM
tfiala added a comment.Dec 7 2015, 9:55 PM

Here's what I'm seeing so far (as of r254983) when running on OS X:

$ test/dotest.py --results-formatter lldbsuite.test.basic_results_formatter.BasicResultsFormatter
Traceback (most recent call last):
  File "test/dotest.py", line 6, in <module>
    import lldbsuite.test
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/__init__.py", line 5, in <module>
    from . import dotest
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/dotest.py", line 43, in <module>
    from . import configuration
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/configuration.py", line 223, in <module>
    __setupCrashInfoHook()
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/configuration.py", line 33, in __setupCrashInfoHook
    test_dir = os.environ['LLDB_TEST']
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/UserDict.py", line 23, in __getitem__
    raise KeyError(key)
KeyError: 'LLDB_TEST'
$ test/dotest.py --curses
Traceback (most recent call last):
  File "test/dotest.py", line 6, in <module>
    import lldbsuite.test
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/__init__.py", line 5, in <module>
    from . import dotest
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/dotest.py", line 43, in <module>
    from . import configuration
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/configuration.py", line 223, in <module>
    __setupCrashInfoHook()
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/configuration.py", line 33, in __setupCrashInfoHook
    test_dir = os.environ['LLDB_TEST']
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/UserDict.py", line 23, in __getitem__
    raise KeyError(key)
KeyError: 'LLDB_TEST'
$ test/dotest.py
Traceback (most recent call last):
  File "test/dotest.py", line 6, in <module>
    import lldbsuite.test
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/__init__.py", line 5, in <module>
    from . import dotest
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/dotest.py", line 43, in <module>
    from . import configuration
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/configuration.py", line 223, in <module>
    __setupCrashInfoHook()
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/configuration.py", line 33, in __setupCrashInfoHook
    test_dir = os.environ['LLDB_TEST']
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/UserDict.py", line 23, in __getitem__
    raise KeyError(key)
KeyError: 'LLDB_TEST'
zturner added a subscriber: zturner.Dec 7 2015, 9:58 PM

Hmm, yea that code path only gets run on OSX. I think the problem is that
I changed the order that happens in. It now happens right when you import
configuration for the first time, and it used to happen explicitly in
main(). So maybe just make that function public, and call in the same
place it used to be called.

To be clear, we explicitly set the LLDB_TEST environment variable as part
of initialization, and it's now trying to read the value of the environment
variable before it's been set. At least I think anyway, I'm going off of
memory, don't have code in front of me.

tfiala added a subscriber: tfiala.Dec 8 2015, 7:58 AM

I think Pavel cleared this up. I'm checking now. If there's anything
remaining, I'll resolve it after I see what happens.

Can this be closed out?

zturner closed this revision.Jan 21 2016, 10:55 AM