These are chat archives for Snaipe/Criterion

19th
Apr 2016
Dominik
@kaidowei
Apr 19 2016 13:08
@Snaipe I'm playing around with the xml output of criterion and it seems, that the xml does not contain the duration. Is that correct or am I missing something?
Franklin Mathieu
@Snaipe
Apr 19 2016 13:11
let me check
It might be something I've forgotten
or simply something not included in the JUnit XML spec
okay, it was forgotten
there is definitely a "time" attribute
For both "testsuite" and "test" it seems
Dominik
@kaidowei
Apr 19 2016 13:14
should I open an issue or create a pullrequest
or do you want to fix it
Franklin Mathieu
@Snaipe
Apr 19 2016 13:14
I'm currently working on the fork patch for nanomsg, so go ahead if you want to fix it
just note that the spec says that the time is an integer representing the number of seconds
so you'll need to round to the nearest integer
Dominik
@kaidowei
Apr 19 2016 13:16
which (in almost all cases) makes no sense :(
Franklin Mathieu
@Snaipe
Apr 19 2016 13:16
yeah, it's a shame
I think the idea was to have more detail in the "timestamp" field
but currently "timestamp" is not an information that is gathered in the stats
actually nevermind, there's no "timestamp_end", so "timestamp" in itself is useless to calculate the time spent
and in any case a timestamp is in seconds
so yeah, it's too bad.
Dominik
@kaidowei
Apr 19 2016 13:18
meaning 0.001 is 1ms
Franklin Mathieu
@Snaipe
Apr 19 2016 13:19
Oh, then that's perfect
I don't know why I remember it being an integer
I must have mixed it up with something else
so then round up to the millisecond
in any case it's the only sane measurement accross platforms, as some does not give precision up to nanoseconds
Dominik
@kaidowei
Apr 19 2016 13:27
elapsed_time is already in ms?
Franklin Mathieu
@Snaipe
Apr 19 2016 13:27
seconds
Dominik
@kaidowei
Apr 19 2016 13:27
ah so, I don't have to do anything, cool
Franklin Mathieu
@Snaipe
Apr 19 2016 13:28
yep
Dominik
@kaidowei
Apr 19 2016 13:28
what does it contain, if the test crashed?
Franklin Mathieu
@Snaipe
Apr 19 2016 13:28
0
because nothing gets reported after the crash
I ought to fix this someday
Dominik
@kaidowei
Apr 19 2016 13:29
the time for a suite... do I have to calc that or is there already a function somewhere?
Franklin Mathieu
@Snaipe
Apr 19 2016 13:29
no, you have to sum all the elapsed_times
Dominik
@kaidowei
Apr 19 2016 13:30
best place for that?
Franklin Mathieu
@Snaipe
Apr 19 2016 13:31
Dominik
@kaidowei
Apr 19 2016 13:42
yeah... are there unittests for the xml io?
Franklin Mathieu
@Snaipe
Apr 19 2016 13:43
there are no unit tests, but a bunch of system tests to validate the output in test/cram
if you want to regenerate the new tests outputs, simply call make cram_tests CRAM=-i
Dominik
@kaidowei
Apr 19 2016 13:51
pullrequest to master or bleeding?
Franklin Mathieu
@Snaipe
Apr 19 2016 13:52
bleeding
I'll roll this out for 2.3.0
Dominik
@kaidowei
Apr 19 2016 14:24
are you currently on bleeding?
can you check for me, if the time is accounted correctly?
I have a sleep in my test and the time is still 0.00
with 2.2.1 it works
Franklin Mathieu
@Snaipe
Apr 19 2016 14:29
mmh, right. Times aren't reported on bleeding for some reason
let me investigate
oh, right, I remember why
that's because the protocol started working with timestamps rather than elapsed times
and currently timestamps aren't implemented
I ought to make an issue so I don't forget
Dominik
@kaidowei
Apr 19 2016 14:35
can you elaborate on the cram tests? can't get them to run
Franklin Mathieu
@Snaipe
Apr 19 2016 14:36
they run alongside everything else when running ctest
if ctest does nothing, then you probably have to run cmake -DCTESTS=ON -DDEV_BUILD=ON .. from your build directory
Dominik
@kaidowei
Apr 19 2016 14:37
ah, I don't know cmake, so I didn't know about ctest
they're running now
where can I see the results of the cram test?
it just says, it failed
Franklin Mathieu
@Snaipe
Apr 19 2016 14:38
oh, right, I'm assuming you don't have cram installed on your system
install cram 0.6 with sudo pip install cram==0.6 (or with --user if you don't want to install it in /usr)
Dominik
@kaidowei
Apr 19 2016 14:39
I have... if I remember correctly, we had this conversation some time ago
Franklin Mathieu
@Snaipe
Apr 19 2016 14:39
okay
in doubt, run make cram_tests
and to troubleshoot with ctest, you need to pass it an option
let me check which one
--output-on-failure
Dominik
@kaidowei
Apr 19 2016 14:40
[100%] Built target criterion_samples
CMake Error at /*beep*/Criterion/.cmake/Modules/Cram.cmake:66 (message):
  Cram tests failed


make[3]: *** [cram_tests] Error 1
make[2]: *** [test/CMakeFiles/cram_tests.dir/all] Error 2
make[1]: *** [test/CMakeFiles/cram_tests.dir/rule] Error 2
make: *** [cram_tests] Error 2
Franklin Mathieu
@Snaipe
Apr 19 2016 14:40
huh.
let me check on my end
oh, it's a bug in the cmake module, somehow
try export PYTHON_BIN=python3
and re-run the tests
I'll push a fix to address the case where PYTHON_BIN isn't set
Dominik
@kaidowei
Apr 19 2016 14:43
yeah, works
Dominik
@kaidowei
Apr 19 2016 15:16
mkay, did my part :)
Franklin Mathieu
@Snaipe
Apr 19 2016 15:17
Thanks!
Dominik
@kaidowei
Apr 19 2016 15:21
uuuh, just noticed, that the locale print sometimes prints "0,05" instead of "0.05"
do you think, xml parsers mind?
Franklin Mathieu
@Snaipe
Apr 19 2016 15:23
oh, right
Dominik
@kaidowei
Apr 19 2016 15:23
damn it
Franklin Mathieu
@Snaipe
Apr 19 2016 15:23
well, according to the standard, xs:decimal needs a dot
actually, let me rephrase: the internet is telling me that the standard says that it needs a dot
Let me check for sure
so how do we fix that?
Franklin Mathieu
@Snaipe
Apr 19 2016 15:27
well, you could swap the locale, but you'll have to restore it afterwards
Dominik
@kaidowei
Apr 19 2016 15:27
that really stinks...
how did you do that for the other output providers?
Franklin Mathieu
@Snaipe
Apr 19 2016 15:28
Other output providers doesn't have this restriction
and I didn't have any floats to print anyway, so that's why
Dominik
@kaidowei
Apr 19 2016 15:29
that's not entirely true, the --tap option also prints floats
Franklin Mathieu
@Snaipe
Apr 19 2016 15:29
I guess the cleanest way (as in, reusable for other output providers) would be to implement a compatibility function for this
yes, but tap doesn't have the restriction, iirc the time is part of a comment string
so using the locale here is the right thing to do
Dominik
@kaidowei
Apr 19 2016 15:30
okay...
the jenkins tap parser had problems finding the time, but I guess that is a problem of the tap-format
(and the locale, maybe)
Franklin Mathieu
@Snaipe
Apr 19 2016 15:31
this is precisely why I'm not testing the timestamps in cram
because the way to print time just isn't consistent
Dominik
@kaidowei
Apr 19 2016 15:31
so the xml.t should be removed? (which is also not a good thing)
Franklin Mathieu
@Snaipe
Apr 19 2016 15:32
no, but xml.t will probably not have time measurements anyway
I mean, time is disabled for tests because you can't really compare two times textually
well, you can, but I mean it's not reliable
because one test might take 1ms to complete one time, and 2ms the other time
Dominik
@kaidowei
Apr 19 2016 15:33
okay... amend this pull request or make another one?
Franklin Mathieu
@Snaipe
Apr 19 2016 15:34
whatever you want, although amending should be simpler for you
Although, its a shame because cram is supposed to support regexp matches on the output
but I haven't been able to get it to work with ANSI escape codes
Dominik
@kaidowei
Apr 19 2016 15:35
yes. but in this case, we need a dot anyway :)
Franklin Mathieu
@Snaipe
Apr 19 2016 15:36
right
Aaah, it is possible to swap locales, nice
Dominik
@kaidowei
Apr 19 2016 15:39
you mean setlocale or something different?
Franklin Mathieu
@Snaipe
Apr 19 2016 15:40
yeah, I didn't know setlocale with NULL gave the current value back
so you should be able to make a locale-specific printf or something of that flavor
Dominik
@kaidowei
Apr 19 2016 15:41
static CR_INLINE or static INLINE
you're using both in the xml file
Franklin Mathieu
@Snaipe
Apr 19 2016 15:42
you can use both, but INLINE is preferred as it's for internal use
Dominik
@kaidowei
Apr 19 2016 15:43
so I'll change static CR_INLINE ?
Franklin Mathieu
@Snaipe
Apr 19 2016 15:43
sure, go ahead
Dominik
@kaidowei
Apr 19 2016 16:35
so, I updated the commit (which was not that easy, if you only used svn all the time :p)
is the pullrequest updated automatically?
Franklin Mathieu
@Snaipe
Apr 19 2016 16:36
yes, the PR mirrors your remote branch
Dominik
@kaidowei
Apr 19 2016 16:37
nice
Franklin Mathieu
@Snaipe
Apr 19 2016 16:41
I commented on your PR
Appart from the two points mentionned, it looks good
Dominik
@kaidowei
Apr 19 2016 16:58
...
g option is not that cool
<testcase name="cmm___item_create" assertions="6" status="PASSED" time="6.03e-05">
Franklin Mathieu
@Snaipe
Apr 19 2016 16:59
ugh, I forgot that it added exponents
Dominik
@kaidowei
Apr 19 2016 17:03
meh, btw. exponents are forbidden in xs:decimal :(
Franklin Mathieu
@Snaipe
Apr 19 2016 17:03
I thought so, but I'm trying to see if there's a way to not have trailing zeroes with %f
and it seems that it's not possible
Dominik
@kaidowei
Apr 19 2016 17:04
but I think that's not a problem...
Franklin Mathieu
@Snaipe
Apr 19 2016 17:04
so screw it, I'll allow the use of %.3f
Dominik
@kaidowei
Apr 19 2016 17:05
or I'll do round(time * 1000) / 1000.0 and %f
Franklin Mathieu
@Snaipe
Apr 19 2016 17:05
sure
Dominik
@kaidowei
Apr 19 2016 17:06
better?
Franklin Mathieu
@Snaipe
Apr 19 2016 17:06
also, is there a reason why this line is using fprintf_locale ?
Dominik
@kaidowei
Apr 19 2016 17:06
because the suite also has a time
entry
Franklin Mathieu
@Snaipe
Apr 19 2016 17:08
right, but the line I linked doesn't
(L164)
I think you meant to change L141
Dominik
@kaidowei
Apr 19 2016 17:09
yeah, just saw that github creates an overlay over the line you linked to...
and yes, you're right
so...
rounding or %f?
%.3f I mean
Franklin Mathieu
@Snaipe
Apr 19 2016 17:11
%.3f, no need to add more complexity
Dominik
@kaidowei
Apr 19 2016 17:32
awesome, just used tap and xml provider in one run... works like a charm.
Franklin Mathieu
@Snaipe
Apr 19 2016 17:50
Merged. Thanks for the PR!
Dominik
@kaidowei
Apr 19 2016 17:50
you're welcome :)