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

Reduce STDERR output during CI #3155

Closed
wants to merge 3 commits into from

Conversation

hangy
Copy link
Member

@hangy hangy commented Mar 29, 2020

Description: The default test output is reduced by only logging error or above to the TAP. If required for debugging, the TAP_LOG_FILTER environment variable can be set to a different level to override it.
Related issues and discussion: #3125

@hangy hangy added the CI Continuous integration label Mar 29, 2020
@hangy hangy self-assigned this Mar 29, 2020
@teolemon
Copy link
Member

fixed a little merge conflict

@stephanegigandet
Copy link
Contributor

@hangy I'm not sure why, but all the traces for the script convert_auchan_data.pl now go to STDERR and it makes the test fail.

RangeError [ERR_CHILD_PROCESS_STDIO_MAXBUFFER]: stderr maxBuffer length exceeded
    at Socket.onChildStderr (child_process.js:386:14)
    at Socket.emit (events.js:311:20)
    at addChunk (_stream_readable.js:294:12)
    at readableAddChunk (_stream_readable.js:271:13)
    at Socket.Readable.push (_stream_readable.js:209:10)
    at Pipe.onStreamRead (internal/stream_base_commons.js:186:23) {
  code: 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER',
  cmd: 'perl -c -CS -Ilib scripts/convert_auchan_data.pl',
  stdout: '',

@hangy
Copy link
Member Author

hangy commented Apr 1, 2020

@hangy I'm not sure why, but all the traces for the script convert_auchan_data.pl now go to STDERR and it makes the test fail.

RangeError [ERR_CHILD_PROCESS_STDIO_MAXBUFFER]: stderr maxBuffer length exceeded
    at Socket.onChildStderr (child_process.js:386:14)
    at Socket.emit (events.js:311:20)
    at addChunk (_stream_readable.js:294:12)
    at readableAddChunk (_stream_readable.js:271:13)
    at Socket.Readable.push (_stream_readable.js:209:10)
    at Pipe.onStreamRead (internal/stream_base_commons.js:186:23) {
  code: 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER',
  cmd: 'perl -c -CS -Ilib scripts/convert_auchan_data.pl',
  stdout: '',

The script probably never worked before. $lc isn't defined in that line:


As to why it never showed an error during CI, I do not know.

@stephanegigandet
Copy link
Contributor

@hangy : with those changes, will we be testing many more things than before? e.g. all the scripts in /scripts maybe ?

"Failing after 131m " --> it will be very problematic if tests take 2 hours.

@hangy
Copy link
Member Author

hangy commented Apr 2, 2020

@hangy : with those changes, will we be testing many more things than before? e.g. all the scripts in /scripts maybe ?

No, not really. We're compiling the scripts in master, too: https://github.com/openfoodfacts/openfoodfacts-server/blob/46b6657/.github/workflows/pull_request.yml#L84
There might be some hidden issue that makes it appear like everything is okay, but I did intent to ensure that scripts compile, as well.

"Failing after 131m " --> it will be very problematic if tests take 2 hours.

Agreed. That's much too long. Simply checking the Perl syntax/compiling the scripts really shouldn't take that long, though. So there is probably some hidden issue that causes too much data to be processed during compilation. I'll have a look ...

Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's the status of this PR. Is it possible to break it in multiple parts?
There seem to be numerous improvements that woul be useful to merge.
But it also makes the tests fail because they are too long, so we can't merge it as is.

@hangy
Copy link
Member Author

hangy commented May 4, 2020

@stephanegigandet currently, I have no idea what might be causing the build to time out. 🤷‍♂️ I'll look into splitting it up, since some of the changes should make sense on their own.

hangy added 2 commits May 5, 2020 23:19
This is done by reducing the default `filter` of the TAP Log::Any adapter. To get back some debug/trace output, it is always possible to run prove the with a different filter:
TAP_LOG_FILTER=trace prove -l
@hangy hangy force-pushed the issue/3125-reduce-test-logs branch from 13a0a47 to 1cb5af0 Compare May 5, 2020 21:20
Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I had misunderstood how to use the TAP_LOG_FILTER variable from the comment line. I think I'll add that to a readme file in the t folder. :)

@VaiTon
Copy link
Member

VaiTon commented Jun 4, 2020

@hangy tests failing

@stephanegigandet
Copy link
Contributor

t/producers.t (Wstat: 512 Tests: 0 Failed: 0)
Non-zero exit status: 2
Parse errors: No plan found in TAP output

-> that's weird, I don't understand why it fails.

@hangy
Copy link
Member Author

hangy commented Jun 4, 2020

-> that's weird, I don't understand why it fails.

Me neither. :) I didn't think I modified the files in that way...

@VaiTon
Copy link
Member

VaiTon commented Jun 26, 2020

@hangy do you still want to merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration 👩‍💻 DevOps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants