Skip to content
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

Report time first for option --duration yes ? #322

Closed
martinmoene opened this issue Sep 3, 2014 · 14 comments
Closed

Report time first for option --duration yes ? #322

martinmoene opened this issue Sep 3, 2014 · 14 comments

Comments

@martinmoene
Copy link
Collaborator

Currently the console reporter reports test durations as:

prompt>TestCatch.exe -r console --duration yes [approx]
Some simple comparisons between doubles completed in 0.000123s
Approximate comparisons with different epsilons completed in 6e-005s
Approximate comparisons with floats completed in 5.7e-005s
Approximate comparisons with ints completed in 5.5e-005s
Approximate comparisons with mixed numeric types completed in 0.000105s
Use a custom approx completed in 0.00016s
Approximate PI completed in 7.2e-005s
===============================================================================
All tests passed (27 assertions in 7 test cases)

The presentation below seems easier to read and to sort:

prompt>TestCatch.exe -r console --duration yes [approx] | sort -n
0.055 ms: Approximate comparisons with ints
0.057 ms: Approximate comparisons with floats
0.06 ms: Approximate comparisons with different epsilons
0.072 ms: Approximate PI
0.105 ms: Approximate comparisons with mixed numeric types
0.123 ms: Some simple comparisons between doubles
0.16 ms: Use a custom approx
===============================================================================
0.632 ms: All tests passed (27 assertions in 7 test cases)

I'm wondering if this would matter to anyone?

@Kosta-Github
Copy link
Contributor

would work for me

maybe also switch from the double representation to uint as milliseconds with a fixed and padded output width?

@jbrwilkinson
Copy link
Contributor

This would replace all the shell scripts we currently use to deduce test timing - bring it on! :-)

@philsquared
Copy link
Collaborator

Yeah, that formatting was a bit tacked on. I'd always meant to revisit.
I'll take a look and keep your suggestions in mind - thanks

@danijar
Copy link

danijar commented May 3, 2015

+1

@p-brz
Copy link

p-brz commented Sep 19, 2016

Someone is working on this already? I may try to implement it if nobody is already doing this.

horenmar added a commit that referenced this issue Feb 17, 2017
Was
"<section-name> completed in XXX s."
Now is
"XXX s: <section-name>"

Closes #322
@horenmar
Copy link
Member

horenmar commented Feb 17, 2017

I took a look at this and ended up with this

0.50439 s: AA
0.512456 s: A
1.00093 s: AB
1.00331 s: A
1.00156 s: BA
1.00743 s: B

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CatchWork.exe is a Catch v1.7.2 host application.
Run with -? for options

-------------------------------------------------------------------------------
B
  BB
-------------------------------------------------------------------------------
<snip>\catchwork\main.cpp(47)
...............................................................................

<snip>\catchwork\main.cpp(49): FAILED:
  REQUIRE( false )

0.173347 s: BB
0 s: B
0.001148 s: B
===============================================================================
test cases:  2 |  1 passed | 1 failed
assertions: 16 | 15 passed | 1 failed

I thought about changing the reported time to milliseconds, but then decided against it -- if your long running tests still finish in lot less than a second, then you probably don't care about how long they actually take.

@p-brz
Copy link

p-brz commented Feb 18, 2017

@horenmar, if you want you can add this snippet (below). I think it ends up looking better:

	void printDuration(double duration){
		const char * unit = "s";

		if((duration * 1000) < 100.0){
			duration *= 1000;
			unit = "ms";
		}

		stream << duration << " " << unit;
	}

@horenmar
Copy link
Member

@paulobrizolara I'd prefer to keep the units uniform, to simplify possible parsing.

@p-brz
Copy link

p-brz commented Feb 24, 2017

Well, i may be wrong, but I don't think this reporter is intended to machine processing.
Instead, i think its targeted for the developer.
So, IMHO the UX should be priority.

Anyway, congratulations for your work. This issue was open for more than 2 years! 😮

@philsquared
Copy link
Collaborator

Thanks @paulobrizolara - @horenmar has been incredible at working through the backlog. See http://www.levelofindirection.com/journal/2017/1/19/catch-up.html for more.

@p-brz
Copy link

p-brz commented Feb 24, 2017

Uou!

I am not using Catch to so many time, but i really likes it!
So i am very happy to see things progressing.

Congrats @philsquared, @horenmar and all community for the work! 👍

@p-brz
Copy link

p-brz commented Feb 24, 2017

Btw, @philsquared , do you think it worths to change the time units format (to something like the initial proposal), besides the possible problems with parsing?

@philsquared
Copy link
Collaborator

I'm in two minds about it. On the one hand I agree that consistency is nice - and makes it easier even for human readers.
On the other hand most unit tests should run in ms, not seconds - and the small fractions look out of place when most, if not all, runtimes are so small. I'm not sure that standardising on ms is much better. And making it a command line option seems overkill. I'm not sure there's an easy answer - and nothing seems significantly better than the status quo (to me at least).

@horenmar
Copy link
Member

horenmar commented Feb 24, 2017

My opinion is that it is not meant for parsing for cases that really matter, but those use-cases could afford to parse ms/s/etc properly anyway. My expected use case is something along the lines of
SelfTest -d yes | sort -n -b, which will print out the longest running tests last, and which would get hopelessly confused if Catch switched between units according to the underlying value.

I agree that the current output could be improved, and I think that changing the output to use fixed format with 2 decimal places would be nicer than the current state, and I'll make the change before the next release, but I don't quite think that dynamic units are that improvement.*

* Standardizing on milliseconds might be an improvement, that depends on how people want to use it -- do they care about which tests take so long, or do they really care about how long each test takes? After all, Catch is not a benchmarking library and should not be used as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants