Make test discovery from file fully lazy #343
Conversation
|
|
||
| stdout, stderr = cmd_output( | ||
| 'python', '-m', 'testify.test_program', 'discovery_error', cwd='examples', | ||
| 'python', '-W', 'ignore::RuntimeWarning:runpy:', |
There was a problem hiding this comment.
curious: what's the RuntimeWarning that's happening here -- can we fix the code instead of silencing the warning?
There was a problem hiding this comment.
It's RuntimeWarning: 'testify.test_program' found in sys.modules after import of package 'testify', but prior to execution of 'testify.test_program'; this may result in unpredic table behaviour and I wasn't quite sure how to restructure testify itself to get around this issue
| T.assert_in('AttributeError: aaaaa!', stderr) | ||
| T.assert_in('AttributeError: aaaaa!', stdout) | ||
|
|
||
|
|
There was a problem hiding this comment.
can we split the lint / test fixes into a separate PR?
There was a problem hiding this comment.
I can do this once we agree that this change should actually be made to Testify
| if self.__name_overrides is None or member.__name__ in self.__name_overrides: | ||
| yield member | ||
| already_inspected = set() | ||
| name_overrides, self.__name_overrides = itertools.tee(self.__name_overrides) |
There was a problem hiding this comment.
I'm -1 on this, I don't believe it makes it any more lazy than the left hand side and at a great cost to readability
| else: | ||
| tests = open(self.filename) | ||
|
|
||
| with tests as tests: |
There was a problem hiding this comment.
now this file doesn't get closed in a timely manner -- is this the RuntimeWarning that you silenced? (seems incorrect to me)
There was a problem hiding this comment.
It is not, but I can close this file.
| for class_path, tests in groupby(tests, lambda test: test[:2]): | ||
| methods = [test.method for test in tests] | ||
| module, cls = class_path | ||
| methods = (test.method for test in tests) |
There was a problem hiding this comment.
given it's already gone through groupby making this into a generator is likely to increase the memory (as it now needs to keep the closure scope alive), and slower because generators
|
The reason I am making this change is not to make Testify itself faster. I would like to reduce the time associated with importing the code under test (~25s?) when repeatedly running groups of tests which has been, up to now, passed in via |
|
ok that makes sense, but there won't be any code reloading so why use a fifo? |
|
Normally testify would exit after each group of tests. I'd like to keep the fifo open and write more tests into the fifo after the first group of tests is done running. |
|
But why? presumably the usual test workflow is:
with your proposal, the changes won't be propagated so it might as well just be
which might as well just be
|
|
A human would not use this feature; this is for running all tests in parallel. |
|
curious what this gains over doing something like split -n5 tests testsplit
ls testsplit* | xargs -n1 -P5 testify ...fifos seem like an overly-complicated solution to splitting a file and parallelizing test running |
|
Note that testify used to have a mode similar to what you're describing but was scrapped due to the sheer complexity: #317 |
|
Right, so there is enough value to solving this problem that another stab at the problem had even been attempted before. Here, something else is doing all the parallelization, so this change is less complicated from the testify point of view. |
Takes #326 to its conclusion (there were still some parts that were not lazy). This allows test methods to be lazily read from a test discovery file.