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

fix: avoid catching init errors and avoid printing their stack #1742

Merged
merged 4 commits into from
Nov 7, 2022

Conversation

kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Nov 5, 2022

Summary:

I experienced the following issue: #1741 and decided to fix it too.

I've made four changes:

  • I removed the try-catch around the body of the init command, to allow errors to bubble up and be handled by the handleError in src/index.ts.
  • I made all the init related errors extend CLIError to avoid printing the stack, since "we know exactly what went wrong" and expects the user to have the means to fix it.
  • Fixed a failing e2e test (due to a trailing \n in the stderr)
  • Added a branch to jest/helpers.ts's handleTestFailure to assert a non-zero exit code if expectedFailure is true. This could have caught this (or a similar issue) earlier.

Test Plan:

I suggest following the instructions for reproducing this on the original issue, only this time, notice how the command exits with status code 1 as expected and it still doesn't print the error stack.

@kraenhansen kraenhansen changed the title Avoid catching init errors and avoid printing their stack fix: avoid catching init errors and avoid printing their stack Nov 5, 2022
@kraenhansen
Copy link
Contributor Author

kraenhansen commented Nov 5, 2022

I fixed the failing e2e test.

@@ -204,6 +204,8 @@ ${chalk.bold('args:')} ${(args || []).join(' ')}
${chalk.bold('stderr:')} ${result.stderr}
${chalk.bold('stdout:')} ${result.stdout}
${chalk.bold('code:')} ${result.code}`);
} else if (options.expectedFailure && result.code === 0) {
throw new Error("Expected command to fail, but it didn't");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could have caught this (or a similar issue) earlier.

@thymikee thymikee linked an issue Nov 7, 2022 that may be closed by this pull request
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@thymikee thymikee merged commit f0f302d into react-native-community:main Nov 7, 2022
thymikee pushed a commit that referenced this pull request Nov 8, 2022
* Defer from "init" to index.ts to handle errors

* Extend a CLIError to avoid printing stack

* Fixing tests by relaxing the stderr match

* Assert non-zero exit code if failure is expected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

init command failure should yield a non-zero exit status code
2 participants