Hi all,
I just put up an optional test results formatter that is a prototype of what we may move towards for our default test summary results. It went in here:
r254530
and you can try it out with something like:
time test/dotest.py --executable pwd/build/Debug/lldb --results-formatter lldbsuite.test.basic_results_formatter.BasicResultsFormatter --results-file st
out
Let me know if this satisfies the basic needs of counts and whatnot. It counts test method runs rather than all the oddball “file, class, etc.” counts we had before.
It prints out the Details section when there are details, and keeps it nice and clean when there are none.
It also mentions a bit about test reruns up top, but that won’t come into play until I get the multi-test-pass, single-worker/low-load mechanism in place, which will depend on newer rerun count awareness support.
The change also cleans up places where the test event framework was using string codes and replaces them with symbolic constants.
Let me know what you think. I can tweak it as needed to address testbot and other needs. Once it looks reasonable, I’d like to move over to using it by default in the parallel test runner rather than the legacy support.
Thanks!
Hi all,
I just put up an optional test results formatter that is a prototype of
what we may move towards for our default test summary results. It went in
here:
r254530
and you can try it out with something like:
time test/dotest.py --executable `pwd`/build/Debug/lldb
--results-formatter
lldbsuite.test.basic_results_formatter.BasicResultsFormatter --results-file
st
out
I cut and paste my line, but more than likely for most people you'd just
want this:
test/dotest.py --results-formatter
lldbsuite.test.basic_results_formatter.BasicResultsFormatter --results-file
stdout
The other stuff was specific to my setup. That line assumes you run from
the lldb source dir root.
Let me know if this satisfies the basic needs of counts and whatnot. It
Also, all the text in the summary is fixed-width lined up nicely, which may not show in the commit message description if you’re using a variable-width font. On a terminal it looks nice.
Can --results-file=stdout be the default so that we don’t have to specify that?
I think it might be already - let me check.
The longer-term goal would be to get this without specifying anything (i.e. does what we want by default). If stdout is not already being used by default when a formatter is specified, that would be an easy fix.
Checking now…
Nope, it didn’t work without the results-file entry.
I can get that fixed up. I’ll look at that now.
Also another stylistic suggestion. I’ve been thinking about how to more logically organize all the source files now that we have a package. So it makes sense conceptually to group all of the different result formatters under a subpackage called formatters. So right now you’ve got lldbsuite.test.basic_results_formatter.BasicResultsFormatter but it might make sense for this to be lldbsuite.test.formatters.basic.BasicResultsFormatter. If you do things this way, it can actually result in a substantially shorter command line, because the --results-formatter option can use lldbsuite.test.formatters as a starting point. So you could instead write:
test/dotest.py --results-formatter basic
dotest then looks for a basic.py module in the lldbsuite.test.formatters package, looks for a class inside with a @result_formatter decorator, and instantiates that.
This has the advantage of making the command line shorter and a more logical source file organization.
Yeah I’d be good with that. I can change that as well.
-Todd
I’ll also move those couple counts (total tests run and the rerun count) under the summary. I missed that when evolving the code.
Yeah I'd be good with that. I can change that as well.
-Todd
Also another stylistic suggestion. I've been thinking about how to more
logically organize all the source files now that we have a package. So it
makes sense conceptually to group all of the different result formatters
under a subpackage called formatters. So right now you've got
lldbsuite.test.basic_results_formatter.BasicResultsFormatter but it
might make sense for this to be
lldbsuite.test.formatters.basic.BasicResultsFormatter. If you do things
this way, it can actually result in a substantially shorter command line,
because the --results-formatter option can use lldbsuite.test.formatters as
a starting point. So you could instead write:
test/dotest.py --results-formatter basic
dotest then looks for a `basic.py` module in the
`lldbsuite.test.formatters` package, looks for a class inside with a
@result_formatter decorator, and instantiates that.
This has the advantage of making the command line shorter *and* a more
logical source file organization.
The other thing that could allow me to do is possibly short-circuit the
results formatter specifier so that, if just the module is specified, and
if the module only has one ResultsFormatter-derived class, I can probably
rig up code that figures out the right results formatter, shortening the
required discriminator to something even shorter (i.e. module.classname
becomes just module.)
Oh yea, I made up that decorator idea because I didn’t know all the formatters were derived from a common base. But your idea is better if everything is derived from a common base. To be honest you could even just generate an error if there are two ResultsFormatter derived classes in the same module. We should be encouraging more smaller files with single responsibility. One of the things I plan to do as part of some cleanup in a week or two is to split up dotest, dosep, and lldbtest.py into a couple different files by breaking out things like TestBase, etc into separate files. So that it’s easier to keep a mental map of where different code is.
When I run this under Python 3 I get “A bytes object is used like a string” on Line 1033 of test_results.py. I’m going to dig into it a little bit, but maybe you know off the top of your head the right way to fix it.
Is there any way to force the single process test runner to use this same system? I’m trying to debug the problem, but this codepath doesn’t execute in the single process test runner, and it executes in the child process in the multiprocess test runner. Basically I need the following callstack to execute in the single process test runner:
Command invoked: C:\Python35\python_d.exe D:\src\llvm\tools\lldb\test\dotest.py -q --arch=i686 --executable D:/src/llvmbuild/ninja_py35/bin/lldb.exe -s D:/src/llvmbuild/ninja_py35/lldb-test-traces -u CXXFLAGS -u CFLAGS --enable-crash-dialog -C d:\src\llvmbuild\ninja_release\bin\clang.exe --results-port 60794 --inferior -p TestIntegerTypesExpr.py D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test --event-add-entries worker_index=7:int
411 out of 412 test suites processed - TestIntegerTypesExpr.py
Traceback (most recent call last):
File “D:\src\llvm\tools\lldb\test\dotest.py”, line 7, in
lldbsuite.test.run_suite()
File “D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\dotest.py”, line 1476, in run_suite
setupTestResults()
File “D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\dotest.py”, line 982, in setupTestResults
results_formatter_object.handle_event(initialize_event)
File “D:\src\llvm\tools\lldb\packages\Python\lldbsuite\test\test_results.py”, line 1033, in handle_event
“{}#{}”.format(len(pickled_message), pickled_message))
TypeError: a bytes-like object is required, not ‘str’
I think I can fix the issue without you debugging.
Getting the single pass test runner to use it isn’t impossible but will take some work. Can you direct-send me the backtrace from the point of failure from your system? Thanks!
-Todd
Can --results-file=stdout be the default so that we don't have to specify
that?
I've adjusted the code here:
r254546
to support dropping the --results-file=stdout part if a --results-formatter
is specified and no results-file is specified. Good idea, thanks!
I think I know how to fix. Trying now.
Yea I was messing around with it too. I don’t have a fix yet but I think you will need to either encode the pickled data as utf8, or better yet, don’t write this:
“{}#{}”.format(…)
because pickled data is supposed to be binary data anyway. So use bytes.append() instead.
Then on the other side in dotest_channels, there’s a couple places where you do something like:
self.ibuffer = “”
which would need to change to
self.ibuffer = b""
and any other similar operations on self.ibuffer which assume string data.
Can you try this?
diff --git a/packages/Python/lldbsuite/test/test_results.py b/packages/Python/lldbsuite/test/test_results.py
index 12fb2e5…547603e 100644
— a/packages/Python/lldbsuite/test/test_results.py
+++ b/packages/Python/lldbsuite/test/test_results.py
@@ -1029,8 +1029,8 @@ class RawPickledFormatter(ResultsFormatter):
Send it as {serialized_length_of_serialized_bytes}#{serialized_bytes}
pickled_message = cPickle.dumps(test_event)
- self.out_file.send(
- “{}#{}”.format(len(pickled_message), pickled_message))
- self.out_file.send(“{}#”.format(len(pickled_message)))
- self.out_file.send(pickled_message)
Can you try making those changes in the other spots? There’s a handful of code you have probably not ever run if you haven’t selected running with a test results formatter.
If not, I can try to address them tonight or tomorrow night when I can get to some kind of python 3 setup.
Can you try making those changes in the other spots? There's a handful of
code you have probably not ever run if you haven't selected running with a
test results formatter.
I'm actually going to make the ibuffer ones now since it's easier for me to
verify that it doesn't break the non-Windows side since I'm right in there
now. If that doesn't do it, we'll need to see what else doesn't work.