cypress run does not log stderr from plugins #5884

santoshyadavdev · 2019-12-05T02:45:51Z

Fixes #5765

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has the original issue been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2019-12-05T02:45:54Z

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

The feature/bugfix is self-documenting from within the product.
The change provides the end user with a way to fix their problem (no dead ends).

Functionality

The code works and performs its intended function with the correct logic.
Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

The code is readable (too many nested 'if's are a bad sign).
Names used for variables, methods, etc, clearly describe their function.
The code is easy to understood and there are relevant comments explaining.
New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
There are comments containing link(s) to the addressed issue (in tests and code).

Quality

The change does not reimplement code.
There's not a module from the ecosystem that should be used instead.
There is no redundant or duplicate code.
There are no irrelevant comments left in the code.
Tests are testing the code’s intended functionality in the best way possible.

Internal

The original issue has been tagged with a release in ZenHub.

jennifer-shehane

Thanks @santoshyadav198613 We will need some tests written to verify this new behavior.

There is a Contributing guide that covers how to contribute and get Cypress running locally in generally here: https://github.com/cypress-io/cypress/blob/develop/.github/CONTRIBUTING.md#L500:L500

Instructions for running the cli tests can be found here: https://github.com/cypress-io/cypress/blob/develop/cli/README.md

The tests for this specific spawn.js file are here: https://github.com/cypress-io/cypress/blob/develop/cli/test/lib/exec/spawn_spec.js#L2:L2

Let us know if you have trouble getting the tests running locally.

santoshyadavdev · 2019-12-05T04:19:11Z

Great will write the test case, thanks.

santoshyadavdev · 2019-12-05T09:26:07Z

Hi @jennifer-shehane ,
While running test case got below erorr:
'BLUEBIRD_DEBUG' is not recognized as an internal or external command,

I am using windows7 and node 12.

flotwig · 2019-12-05T16:41:24Z

While running test case got below erorr:
'BLUEBIRD_DEBUG' is not recognized as an internal or external command,

I am using windows7 and node 12.

Try pulling this branch down, #5888 was just merged and that should fix the issue you're experiencing on Windows.

santoshyadavdev · 2019-12-05T19:55:25Z

Hi @flotwig and @jennifer-shehane ,
Thanks for all the help, added the test case, let me know if anything else is needed.

flotwig · 2019-12-05T21:44:59Z

Great, I want to do some testing and make sure it doesn't cause us to leak any extra stderr before this merges.

Test added, so dismissing my previous review.

flotwig

I tried testing this by merging it into the PR for #5269 and no stderr was captured for 1_system_node_spec. onStderrData gets passed as filterFn here:

cypress/cli/lib/exec/spawn.js

Lines 34 to 47 in 004067c

    
           function shouldShowStderrLine (line, filterFn) { 
        
             // bail if this is warning line garbage 
        
             if (isGarbageLineWarning(line)) { 
        
               return false 
        
             } 
        
             // if we have a callback and this explictly returns 
        
             // false then bail 
        
             if (filterFn && filterFn(line) === false) { 
        
               return false 
        
             } 
        
             return true 
        
           }

So this code change doesn't make a difference. I think that what needs to happen is, if debugElectron is enabled, filter out the logs that are produced by electron and send them to debugElectron(line), otherwise, send them directly to stderr.

santoshyadavdev · 2019-12-09T16:50:56Z

Cool @flotwig Will check it , will let you know if I need any help.

santoshyadavdev · 2019-12-17T18:36:37Z

Hi @flotwig ,
Need one help here, I can see different test case getting failed.

jennifer-shehane · 2020-01-10T06:51:59Z

@santoshyadav198613 Hey, do you still need help with this PR? I do not see any failing tests currently in CI, was there some staged code that you had questions about or is this ready for rereview?

santoshyadavdev · 2020-01-10T07:03:20Z

Hey @jennifer-shehane, yeah looks like CI is green, can you help with the review.

flotwig · 2020-01-29T16:59:43Z

cli/lib/exec/spawn.js

@@ -251,8 +251,12 @@ module.exports = {
            // the stderr logs unless we've explicitly
            // enabled the electron debug logging
            if (!debugElectron.enabled) {
+              debugElectron(str)


This doesn't make sense - "if the Electron debugger is disabled, then log this using the Electron debugger"?

flotwig · 2020-01-29T17:29:11Z

cli/lib/exec/spawn.js

@@ -251,8 +251,12 @@ module.exports = {
            // the stderr logs unless we've explicitly
            // enabled the electron debug logging
            if (!debugElectron.enabled) {


Since there was a PR to ignore garbage coming from Electron #4644, I'm almost wondering if we can just get rid of lines 253-259 and return true here. Theoretically, all other stderr should either be from the plugin, or from a genuine error in Cypress.

There's a problem on Linux, where we always log Electron's Chromium stderr by enabling , which isn't filtered out:

cypress/cli/lib/exec/spawn.js

Lines 247 to 256 in 719fd98

const userFriendlySpawn = (linuxWithDisplayEnv) => {

debug('spawning, should retry on display problem?', Boolean(linuxWithDisplayEnv))

let brokenGtkDisplay

const overrides = {}

if (linuxWithDisplayEnv) {

_.extend(overrides, {

electronLogging: true,

cypress/cli/lib/exec/spawn.js

Lines 158 to 160 in 719fd98

if (electronLogging) {

stdioOptions.env.ELECTRON_ENABLE_LOGGING = true

}

So if a regex could be added to GARBAGE_WARNINGS to filter Electron's Chromium stderr, we can just remove lines 253-259 in spawn.js.

We still need to keep these Chromium stderrs though, because we rely on them to know why Cypress crashed in some situations:

cypress/cli/lib/exec/spawn.js

Lines 260 to 262 in 719fd98

if (util.isBrokenGtkDisplay(str)) {

brokenGtkDisplay = true

}

@santoshyadav198613 Do you want to try to implement this, or would you prefer me to do it?

I am traveling till 8th Feb, I can take it after that. But if you think it's urgent, please go ahead.

flotwig

Thanks for opening this PR!

jennifer-shehane · 2020-02-18T09:49:53Z

Hey @santoshyadav198613, will you have time to address the changes asked of this PR? We will have to close this due to inactivity otherwise.

santoshyadavdev · 2020-02-18T16:33:42Z

Sorry for delay, will get the issues sorted tomorrow.

santoshyadavdev · 2020-02-19T11:04:08Z

Hi @jennifer-shehane ,
updated as per review comments.

Fixes cypress-io#5765

flotwig

@santoshyadav198613 it looks like the comment here hasn't been addressed:

#5884 (comment)

to clarify, we want to hide the Electron Chromium debug output. With the change you've made, all stderr from the Electron process will be printed, which is good. But it means we will also get lots of log lines in a format like this:

[25129:25129:0220/105230.858006:ERROR:vaapi_wrapper.cc(417)] vaInitialize failed: unknown libva error

These come from the Chromium process inside of Electron. There needs to be a regex added to GARBAGE_WARNINGS to block these lines from getting printed out with the rest of the stderr. They should be sent to debugElectron instead, so if somebody really needs to see these logs, they can by using DEBUG=cypress:electron. Otherwise, all Linux users will see tons of garbage from Electron in their stderr.

jennifer-shehane · 2020-03-10T05:49:06Z

Hey @santoshyadav198613, will you have time to address the changes mentioned here #5884 (review)? We will have to close this due to inactivity otherwise.

jennifer-shehane · 2020-03-17T07:23:36Z

Closing due to inactivity. Please reopen the PR or open a new PR to address the changes required for this issue.

jennifer-shehane requested a review from flotwig December 5, 2019 03:58

jennifer-shehane previously requested changes Dec 5, 2019

View reviewed changes

santoshyadavdev force-pushed the Issue-5765-cypress-run-does-not-log-stderr branch from ab32c7a to 3e742ed Compare December 5, 2019 19:33

santoshyadavdev requested a review from jennifer-shehane December 5, 2019 20:19

flotwig suggested changes Dec 9, 2019

View reviewed changes

santoshyadavdev force-pushed the Issue-5765-cypress-run-does-not-log-stderr branch from adf047b to a66d09a Compare December 17, 2019 17:42

jennifer-shehane requested a review from flotwig January 13, 2020 09:38

flotwig reviewed Jan 29, 2020

View reviewed changes

flotwig suggested changes Jan 29, 2020

View reviewed changes

santoshyadavdev force-pushed the Issue-5765-cypress-run-does-not-log-stderr branch from a66d09a to 711100f Compare February 19, 2020 11:03

santoshyadavdev force-pushed the Issue-5765-cypress-run-does-not-log-stderr branch from 711100f to 7fc0b81 Compare February 19, 2020 16:44

cypress run does not log stderr from plugins

fa2b0d4

Fixes cypress-io#5765

santoshyadavdev force-pushed the Issue-5765-cypress-run-does-not-log-stderr branch from 7fc0b81 to fa2b0d4 Compare February 19, 2020 16:46

jennifer-shehane requested review from flotwig and removed request for jennifer-shehane February 20, 2020 10:51

flotwig suggested changes Feb 20, 2020

View reviewed changes

jennifer-shehane closed this Mar 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cypress run does not log stderr from plugins #5884

cypress run does not log stderr from plugins #5884

santoshyadavdev commented Dec 5, 2019 •

edited by brian-mann

cypress-bot bot commented Dec 5, 2019

jennifer-shehane left a comment

santoshyadavdev commented Dec 5, 2019

santoshyadavdev commented Dec 5, 2019 •

edited

flotwig commented Dec 5, 2019

santoshyadavdev commented Dec 5, 2019

flotwig commented Dec 5, 2019

flotwig left a comment

santoshyadavdev commented Dec 9, 2019

santoshyadavdev commented Dec 17, 2019

jennifer-shehane commented Jan 10, 2020

santoshyadavdev commented Jan 10, 2020

flotwig Jan 29, 2020

flotwig Jan 29, 2020

flotwig Jan 29, 2020

flotwig Jan 29, 2020

santoshyadavdev Jan 29, 2020 •

edited

flotwig left a comment

jennifer-shehane commented Feb 18, 2020

santoshyadavdev commented Feb 18, 2020

santoshyadavdev commented Feb 19, 2020

flotwig left a comment

jennifer-shehane commented Mar 10, 2020

jennifer-shehane commented Mar 17, 2020

	function shouldShowStderrLine (line, filterFn) {
	// bail if this is warning line garbage
	if (isGarbageLineWarning(line)) {
	return false
	}

	// if we have a callback and this explictly returns
	// false then bail
	if (filterFn && filterFn(line) === false) {
	return false
	}

	return true
	}

	const userFriendlySpawn = (linuxWithDisplayEnv) => {
	debug('spawning, should retry on display problem?', Boolean(linuxWithDisplayEnv))

	let brokenGtkDisplay

	const overrides = {}

	if (linuxWithDisplayEnv) {
	_.extend(overrides, {
	electronLogging: true,

	if (electronLogging) {
	stdioOptions.env.ELECTRON_ENABLE_LOGGING = true
	}

cypress run does not log stderr from plugins #5884

cypress run does not log stderr from plugins #5884

Conversation

santoshyadavdev commented Dec 5, 2019 • edited by brian-mann

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Dec 5, 2019

PR Review Checklist

User Experience

Functionality

Maintainability

Quality

Internal

jennifer-shehane left a comment

Choose a reason for hiding this comment

santoshyadavdev commented Dec 5, 2019

santoshyadavdev commented Dec 5, 2019 • edited

flotwig commented Dec 5, 2019

santoshyadavdev commented Dec 5, 2019

flotwig commented Dec 5, 2019

flotwig left a comment

Choose a reason for hiding this comment

santoshyadavdev commented Dec 9, 2019

santoshyadavdev commented Dec 17, 2019

jennifer-shehane commented Jan 10, 2020

santoshyadavdev commented Jan 10, 2020

flotwig Jan 29, 2020

Choose a reason for hiding this comment

flotwig Jan 29, 2020

Choose a reason for hiding this comment

flotwig Jan 29, 2020

Choose a reason for hiding this comment

flotwig Jan 29, 2020

Choose a reason for hiding this comment

santoshyadavdev Jan 29, 2020 • edited

Choose a reason for hiding this comment

flotwig left a comment

Choose a reason for hiding this comment

jennifer-shehane commented Feb 18, 2020

santoshyadavdev commented Feb 18, 2020

santoshyadavdev commented Feb 19, 2020

flotwig left a comment

Choose a reason for hiding this comment

jennifer-shehane commented Mar 10, 2020

jennifer-shehane commented Mar 17, 2020

santoshyadavdev commented Dec 5, 2019 •

edited by brian-mann

santoshyadavdev commented Dec 5, 2019 •

edited

santoshyadavdev Jan 29, 2020 •

edited