These are chat archives for Snaipe/Criterion

15th
Mar 2016
Dominik
@kaidowei
Mar 15 2016 12:05
@Snaipe a coworker found another interesting thing
[====] Running 6 tests from confparse_complex:
[RUN ] confparse_complex::confcopy_ar7cfg
[RUN ] confparse_complex::confcopy_ifaceconfig
[ERR ] Could not link back the event PID to the active workers.
[ERR ] The event pipe might have been corrupted.
only occurs with -jX (X>1) with cr_assert_file_contents_eq_str()
and not always, but with about 50% chance
currently, we do not have a minimal working example, but I can test for you
the function that provokes the error:
static void verify_complex_config(struct ar7cfg **cfg)
{
    OUTSTREAM *os;
    FILE *fexpected;

    static char saved[8<<20];

    cr_assert_not_null(saved);
    os = outstream_mem(saved, sizeof(saved));
    cr_assert_not_null(os);

    /* Update the tests whenever the defaults in ar7cfg.tab change */
    ASSERT_EQUAL((*cfg)->mode, dsldmode_router);
    ASSERT_STRING_EQUAL((*cfg)->active_provider, "tonline");
    ASSERT_EQUAL((*cfg)->mtu_cutback, 1500);

    config_varsave_topsection_ostream(AR7CFG_ar7cfg_struct, os, (void **)cfg, AR7CFG_ar7cfg_struct->offset, 0);

    fexpected = fopen("expected", "r");

    //~ cr_assert(!feof(fexpected) && !ferror(fexpected));
    cr_assert_file_contents_eq_str(fexpected, saved);

    fclose(fexpected);
    outstream_close(os);
}
Franklin Mathieu
@Snaipe
Mar 15 2016 12:08
Might be the result of an out of bounds -- I would gladly accept the example so I can get myself to debug
Dominik
@kaidowei
Mar 15 2016 12:30
as I said, no working minimal example...
sorry, can't reproduce it without our complete code...
Dominik
@kaidowei
Mar 15 2016 12:38
Update:
if we call cr_file_match_str everything works fine, but the macro seems to cause the problem
when we use cr_assert_file_contents_neq_strinstead of cr_assert_file_contents_eq_strthe string parameter is cropped
in the resulting error message
and the 8<<20 is not a problem. Even if we malloc the buffer, criterion still fails
Franklin Mathieu
@Snaipe
Mar 15 2016 12:42
It seems very similar to the out of bounds I fixed for 2.2.1 -- it's possible that this is related and that I missed the issue.
Dominik
@kaidowei
Mar 15 2016 12:44
we should think about a bug hunting bounty program ;)
Franklin Mathieu
@Snaipe
Mar 15 2016 12:50
I can only pay in coffee at the moment though :D
But yeah, these bugs are the result of me rushing 2.0
Rushing a release never does well for a project.
Dominik
@kaidowei
Mar 15 2016 12:56
tell me about it... our PM is rushing all the time :p
Dominik
@kaidowei
Mar 15 2016 13:11
it's okay, we have coffee here too :)
update: if we provide a custom message for the assert, there is no problem (seems reasonable)
maybe there is a problem with the pipe and (very long) strings
he says without the send_event there is no problem
Franklin Mathieu
@Snaipe
Mar 15 2016 13:14
right
I think that since saved is passed through the pipe with the default message
it just tries to handle it and dies
or rather than dying, corrupts the pipe
Dominik
@kaidowei
Mar 15 2016 13:15
a bit strange, that this only happens in 50% of the cases
Franklin Mathieu
@Snaipe
Mar 15 2016 13:16
it really depends on the state of the pipe tbh
if the pipe is almost full and you try to push 8 << 20 bytes down its throat, it might lead to this particular problem
which is probably why you can't reproduce it in a minimal example, there needs to be other tests run in parallel that push data down the pipe
this is all supposition though, I'll have to make some tests on my end
I'm curious if this would happen with the I/O rewrite on bleeding, though
Dominik
@kaidowei
Mar 15 2016 13:18
...
good idea with the multiple tests
Dominik
@kaidowei
Mar 15 2016 13:26
does that "work" for you too=
Franklin Mathieu
@Snaipe
Mar 15 2016 13:26
Can't really run this right now, I'll tell you when I get a bit more time
I assume that the issue disappears when you reduce the side of saved?
Dominik
@kaidowei
Mar 15 2016 13:27
yeah and sorry, you need a file with a looong string in "testfile"
Franklin Mathieu
@Snaipe
Mar 15 2016 13:27
er, buf
Dominik
@kaidowei
Mar 15 2016 13:27
let me check
yes
reducing the buf size and the file content helps
Franklin Mathieu
@Snaipe
Mar 15 2016 13:30
file contents shouldn't really matter (other than speeding up the test), because the file name is passed rather than the full blown contents
Dominik
@kaidowei
Mar 15 2016 13:30
well, the test does not work if the buffer is smaller than the test :p
Franklin Mathieu
@Snaipe
Mar 15 2016 13:30
right, indeed
Although I think that it actually would happen with cr_assert_str_eq
since both sides are passed to the pipe
Dominik
@kaidowei
Mar 15 2016 13:31
is the assert always in the root process?
Franklin Mathieu
@Snaipe
Mar 15 2016 13:32
and if one of the strings exceed the size of the internal pipe buffer, it might get confused
the assert is done in the child process
but gets reported, along with relevant data, to the parent
Dominik
@kaidowei
Mar 15 2016 13:32
even if it doesn't fail?
Franklin Mathieu
@Snaipe
Mar 15 2016 13:32
Yes, because of statistics
+ support of report formats like JUnit XML
The issue is weird though -- I think that the large message gets split, somehow
if my memory of man pipe serves well, this shouldn't happen
Dominik
@kaidowei
Mar 15 2016 13:36
just a normal ubuntu 14.04
and the coworkers system is linux mint version... I don't know
Franklin Mathieu
@Snaipe
Mar 15 2016 13:38
it's more probable that I screwed up the message passing in a very subtle way
but in the meantime, consider using explicit assert messages for these assertions
Dominik
@kaidowei
Mar 15 2016 13:38
yes, we're doing that already
Franklin Mathieu
@Snaipe
Mar 15 2016 13:38
otherwise you'll also get hit by the cost of transfering a +8 << 20 bytes assert message
I wonder
maybe the default message for these assertions isn't the best choice
the default message could only pass a minimal diff of both data
Dominik
@kaidowei
Mar 15 2016 13:41
yeah, catting the whole file might not be the best idea
diff is a cool idea
but relies on external libs/programs
Franklin Mathieu
@Snaipe
Mar 15 2016 13:42
also, it's hardly an inline display
plus, it assumes that the file/strings aren't binary data
Dominik
@kaidowei
Mar 15 2016 13:42
right
Franklin Mathieu
@Snaipe
Mar 15 2016 13:43
which also makes me realize that there should really be assertion functions to compare a file to a byte array
not all files are text
Dominik
@kaidowei
Mar 15 2016 13:44
:+1: