-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for sorting table output by cover, elapsed #70
Conversation
These functions were redundant vs. summary.GetSortedPackages(). Remove the redundant sort so that we can configure the sorting mechanism in exactly one place. Signed-off-by: Joe Stringer <joe@wand.net.nz>
@@ -28,6 +28,7 @@ var ( | |||
fileNamePtr = flag.String("file", "", "") | |||
formatPtr = flag.String("format", "", "") | |||
followPtr = flag.Bool("follow", false, "") | |||
sortPtr = flag.String("sort", "name", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on changing this to sort-pkg
, so it's obvious which table is being sorted?
I could see a future world where someone requests sort-test
(name, elapsed, etc.) or even sort-bench
(name, CPU, memory, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like adding specifics for pkg,test,bench forces the user to worry more about the specific output they get and which particular type of output should be sorted in which way. It would be simpler to have the user specify one parameter they care about the most based on their use case of what they want to analyze & optimize.
That said, some sorting preferences may not make sense particular test data, like sorting by coverage won't make sense for benchmarks and the cpu/memory/allocs won't make sense for regular tests. Hmm 🤔
Personally I'd be fine if it just has one sort option. Then if I pass the wrong sort function depending on the data, it just formats in some standard sorting order like by name. For instance passing benchmark data into tparse -sort cover
if it just fell back to sorting by name. Or if I pass regular test data into tparse -sort cpu
if it also just fell back to sorting by name. The output table wouldn't have that column, so it would become clear pretty quickly that I just specified a nonsense sorting order.
Another option would be to support a slice of sorting functions, so you could pass any data into tparse -sort cpu,cover,name
and then for benchmarks it'll sort by CPU, for packages it'll sort by coverage, then everything falls back to name as the tiebreak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfridman what do you think, do you prefer different sort parameters for packages/benchmarks/tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was noodling on this for a bit. I quite like the general direction of how this is composed.
So okay merging once the comments are addressed.
There is an outstanding question of how to sort the tests table, but we can cross that bridge later. So far sorting that table by elapsed has been sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was also wondering about the role of the tests table vs. the "-follow" output that demonstrates progress for local tty. Personally for cilium/cilium I'll probably just print the package summaries like this:
https://app.travis-ci.com/github/cilium/cilium/jobs/572768127
I wouldn't mind dropping the TEST pkg/foo
lines from the makefile in the CI as well, but that's probably more of an item for me. I guess it depends on how the discussion about a -ci
flag goes. When testing locally, I'd like ongoing feedback that tests are being run. In CI, that doesn't matter (and actually becomes counterproductive, because those nice formatting progress updaters tend to end up spamming test logs in CI). With a flag to enable/disable a progress indicator and an env variable I should be able to achieve this pretty easily in cilium/cilium.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests table can be output with -all
(which is -pass
and -skip
combined) and doesn't have much to do with -follow
. Although for very large projects, I feel this table is only relevant if you're interested in capturing the slowest N tests per package. E.g., tparse -all -slow 5
means show the table tests and only display the slowest 5.
The -follow
was requested quite a bit because folks wanted to follow the output of go test
despite running it with -json
. That output often has t.Log
or equivalent output, which is not captured in the table test.
Not sure I understand this bit, but feedback is welcome 😄
I wouldn't mind dropping the TEST pkg/foo lines from the makefile in the CI as well, but that's probably more of an item for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't mind dropping the TEST pkg/foo lines from the makefile in the CI as well, but that's probably more of an item for me.
Not sure I understand this bit, but feedback is welcome 😄
In CI when the tests are running, it would be nice that each time the testsuite starts testing a new package, it prints one line with the package. This way, if one test gets stuck then that test is the last test that you see in the test output. So it's easy to tell which test you should look at. On the other hand if there is no output until the end when the table is printed, then it can be hard to tell which package the CI got stuck on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can open a new issue to keep the conversation open, otherwise, this will likely be lost in this thread.
Curious, is the combination of -all -follow
not working for you? Example:
go test -v -count=1 crypto -json | tparse -all -follow
The -follow
prints all go test output as if you hadn't run with -json
, like this:
go test -v -count=1 crypto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the problem with that is that for large testsuites it becomes too noisy. It prints N lines per test in the codebase, which is much larger than the number of packages. Too much output then becomes difficult to parse through again. I like the summary-only format, but that sometimes makes it hard to see which test hit problems if one of the tests just hangs rather than completing.
I can open another issue to discuss this further, maybe I can try to formulate my thoughts a bit more cohesively there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'll be great, I work(ed) with fairly large code bases with loads of output so I think I understand where you're getting at. There is probably some middle ground between no output and all, but I just haven't been able to find the right balance so input appreciated.
Refactor and abstract the package sorting functions so that the next commit can add new methods to sort the table output. No functional changes in this commit. Signed-off-by: Joe Stringer <joe@wand.net.nz>
Signed-off-by: Joe Stringer <joe@wand.net.nz>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Thanks for putting this through!
Add a new
-sort
flag with the optionsname
(default),cover
,elapsed
so that you can easily see test results ordered by these other attributes:Tested using variations on the following commands under https://github.com/cilium/cilium/:
See individual commits for more details: