Closed Bug 957410 Opened 7 years ago Closed 7 years ago

|mach valgrind-test| doesn't detect failures in child processes


(Testing :: General, defect)

Not set


(Not tracked)



(Reporter: n.nethercote, Assigned: n.nethercote)



(1 file, 1 obsolete file)

|mach valgrind-test| currently uses Valgrind's --error-exitcode=1 flag to detect if Valgrind finds any errors, in which case it returns an appropriate exit code.

However, this only works if Valgrind finds errors in the initial process. Errors in child processes aren't detected. This isn't such a big deal now, but will be in the future with e10s.

The only solution I can see is to scan Valgrind's output, looking for the presence of "ERROR SUMMARY" lines with non-zero error counts. This is what TBPL does
( in order to highlight these lines in TBPL output. But in order to turn the jobs red we need a non-zero exit code, so I think |mach valgrind-test| needs to do likewise.
OS: Linux → All
Hardware: x86_64 → All
Here's an attempt that doesn't work. I cargo-culted the test_output/handler
stuff from testing/mochitest/, but I don't understand it at all
and it doesn't work -- NonZeroErrorSummaryFilter.filter doesn't seem to get
called at all.

gps, what is the right way to do this?
Assignee: nobody → n.nethercote
Attachment #8356904 - Flags: feedback?(gps)
Depends on: 945427
Comment on attachment 8356904 [details] [diff] [review]
Make |mach valgrind-test| detect errors in child processes.

Review of attachment 8356904 [details] [diff] [review]:

mach's logging internals are crap and have been in need of a refactor.

This likely isn't working because the "unstructured" (classical) logging a) isn't being converted to structured logging or b) that structured logger's output isn't being captured.

b can be solved by calling log_manager.register_structured_logger('name.of.logger'). I have no clue what logger is being used behind the scenes here. But if you register the root logger (logging.getLogger()), it should catch it.
Attachment #8356904 - Flags: feedback?(gps) → feedback+
Ok, the mochitest code was a red herring. The mochitests produced logging output, but Valgrind just writes non-logging output to stderr, so they're not the same thing.

To summarize the situation and what I want...

- Valgrind's output currently goes to stderr, i.e. it's printed to the terminal.

- I want to still print it to stderr, but I also want to parse the output in order to pick out some interesting bits.

- So I probably need to capture the output in some fashion -- in a buffer, or a file?

- Valgrind has options that allow its output to be sent to a file or an fd (or even a socket) instead of stderr.

- Ideally the printing of the output would happen in real-time, rather than being done in a single hit at the end -- this is useful for people running it locally.
> - Ideally the printing of the output would happen in real-time, rather than
> being done in a single hit at the end -- this is useful for people running
> it locally.

If I'm willing to give up this requirement, it's not hard -- I can just use Valgrind's --log-file option and then print and parse the output at the end. Hmm.
Maybe I can use --log-fd and set up a tee-like fd that both prints the output and captures it into a buffer/file?
mozrunner redirects stderr to stdout, so you shouldn't have to jump through hoops here. You probably just want to do something similar to what Mochitest is doing:

outputHandler is just a callable, so you can pass a function there, Mochitest is using a class with a __call__ method so it can store some extra stuff, but a closure is probably sufficient.

That callback should get called for every line of stderr/stdout output.
Thanks, Ted! That works nicely.
Rather than relying entirely on the exit code to determine the test's outcome,
we now look at the output to determine if errors occurred, and look at the exit
code only to determine if something when wrong (such as Valgrind crashing).
Attachment #8364850 - Flags: review?(gps)
Attachment #8356904 - Attachment is obsolete: true
Attachment #8364850 - Flags: review?(gps) → review+
No longer depends on: 945427
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.