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(openapi): return rejected Promise if spec uploads fail #409

Merged
merged 4 commits into from
Dec 17, 2021

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Dec 17, 2021

🧰 Changes

Turns out we had an edge case where we were returning a console.log() rather than a rejected Promise when we received errors from the API, resulting in an incorrectly successful exit code. This PR fixes that in 0f2372e so we properly return an exit code of 1. And as expected, some housekeeping:

  • Refactors existing tests to check for the rejected Promise rather than a console output 8811c8c
  • Adds additional test coverage to check for our other API error edge cases. We're now over 90% line coverage in the OpenAPI command file! 📈 8811c8c
  • Slightly reformats our rejected Promise error outputs to be padded with newlines 9936de5
  • Small fix to our general command handler so it properly uses our lengthier error message for invalid commands. 3b75d29 See below for a before and after:

image

Resolves #407.

🧬 QA & Testing

If the tests pass, we should be good! But as an extra check, you can check out this branch and run the following commands to confirm that we see a non-zero exit code:

$ bin/rdme openapi --id="asdf" --key="asdf" ./__tests__/__fixtures__/ref-oas/petstore.json

We couldn't find your API key.

If you need help, email [email protected] and include the following link to your API log: 'https://docs.readme.com/logs/xxx-xxx'.

$ echo $?
1

- Refactored tests to check for promise rejections and to inspect error objects, rather than checking for console.log outputs

- Also added tests for edge cases (i.e. non-JSON error messages, spec timeout errors)
We were doing this in the console logging that I removed in 0f2372e, but I liked that formatting so I'm just having us do that globally.
@kanadgupta kanadgupta requested a review from erunion December 17, 2021 22:47
@erunion erunion added the bug Something isn't working label Dec 17, 2021
Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

can't believe we weren't throwing an exit(1) for failed uploads 😞

@erunion erunion merged commit e282b54 into main Dec 17, 2021
@erunion erunion deleted the fix/reject-promise-for-spec-upload-failures branch December 17, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-zero exit status on failure
2 participants