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

Bug fix: Report launcher process error when the process exit event is not emitted #3647

Conversation

chrisbottin
Copy link
Contributor

This PR is to ensure the launcher process error is reported in the logs when the process exit event is not emitted which can happen in some cases.

As per described on this page https://nodejs.org/api/child_process.html#child_process_event_error, "The 'exit' event may or may not fire after an error has occurred. When listening to both the 'exit' and 'error' events, guard against accidentally invoking handler functions multiple times."

Without this fix, if the browser binary file is missing, Karma will report the following if only in Debug mode:

DEBUG [launcher]: Process {{browser_name}} exited with code -1

This is not really helpful to the users and if debug log level is not set, they won't see this message.

With this fix, if the browser binary file is missing, Karma will always report the following:

ERROR [launcher]: Cannot start {{browser_name}}
        Can not find the binary {{path_to_binary}}
        Please set env variable {{env_cmd}}

This error is clear to the users and always visible

@google-cla
Copy link

google-cla bot commented Feb 3, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@AppVeyorBot
Copy link

Build karma 2918 failed (commit f5b78c5937 by @)

@karmarunnerbot
Copy link
Member

Build karma 521 failed (commit f5b78c5937 by @)

@karmarunnerbot
Copy link
Member

Build karma 520 failed (commit f5b78c5937 by @)

@chrisbottin chrisbottin force-pushed the fix-report-launcher-process-error branch from c7f1c04 to d42e6cc Compare February 3, 2021 14:48
@chrisbottin
Copy link
Contributor Author

@googlebot I signed it!

@karmarunnerbot
Copy link
Member

Build karma 522 completed (commit cdc8f51f3c by @)

@AppVeyorBot
Copy link

Build karma 2919 completed (commit cdc8f51f3c by @)

@karmarunnerbot
Copy link
Member

Build karma 521 completed (commit cdc8f51f3c by @)

@chrisbottin chrisbottin force-pushed the fix-report-launcher-process-error branch from d42e6cc to 12491c8 Compare February 3, 2021 15:59
@AppVeyorBot
Copy link

Build karma 2920 completed (commit a5d35acf91 by @)

@karmarunnerbot
Copy link
Member

Build karma 523 completed (commit a5d35acf91 by @)

@karmarunnerbot
Copy link
Member

Build karma 522 completed (commit a5d35acf91 by @)

Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Thanks, one fix up please.

lib/launchers/process.js Show resolved Hide resolved
@chrisbottin chrisbottin force-pushed the fix-report-launcher-process-error branch from 12491c8 to f896323 Compare February 3, 2021 19:34
@AppVeyorBot
Copy link

Build karma 2921 completed (commit c4bdbd38a5 by @)

@karmarunnerbot
Copy link
Member

Build karma 524 completed (commit c4bdbd38a5 by @)

@karmarunnerbot
Copy link
Member

Build karma 523 completed (commit c4bdbd38a5 by @)

@johnjbarton johnjbarton merged commit 7ab86be into karma-runner:master Feb 3, 2021
@chrisbottin
Copy link
Contributor Author

@johnjbarton I see the build on master is failing commit message validation due to the fact the pull request ID was appended to the commit message on merge.

Original Commit Message - 65 chars: fix: report launcher process error when exit event is not emitted
Merged Commit Message - 73 chars: fix: report launcher process error when exit event is not emitted (#3647)
Commit Message Length Limit set at 72 chars on branches and master

What can we do about this?

@chrisbottin
Copy link
Contributor Author

@johnjbarton Potential fix => #3650

karmarunnerbot pushed a commit that referenced this pull request Feb 12, 2021
## [6.1.1](v6.1.0...v6.1.1) (2021-02-12)

### Bug Fixes

* **config:** check extension before ts-node register ([#3651](#3651)) ([474f4e1](474f4e1)), closes [#3329](#3329)
* report launcher process error when exit event is not emitted ([#3647](#3647)) ([7ab86be](7ab86be))
@karmarunnerbot
Copy link
Member

🎉 This PR is included in version 6.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

This was referenced Mar 15, 2021
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
…rma-runner#3647)

Co-authored-by: Chris Bottin <cbottin@smartcommunications.com>
anthony-redFox pushed a commit to anthony-redFox/karma that referenced this pull request May 16, 2023
## [6.1.1](karma-runner/karma@v6.1.0...v6.1.1) (2021-02-12)

### Bug Fixes

* **config:** check extension before ts-node register ([karma-runner#3651](karma-runner#3651)) ([474f4e1](karma-runner@474f4e1)), closes [karma-runner#3329](karma-runner#3329)
* report launcher process error when exit event is not emitted ([karma-runner#3647](karma-runner#3647)) ([7ab86be](karma-runner@7ab86be))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants