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

test(message-utils): add message-utils testing #3212

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

rwaskiewicz
Copy link
Member

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

There's no unit testing around the message utils we have, which came up in #3211. #3211 did a lot of explicit type annotation around passing any into our message-utils#catchError method, so I figured I'd start there in introducing some tests.

GitHub Issue Number: N/A

What is the new behavior?

  • add tests to catchError
  • fix cases where we could print a zero-len string
    • it's still not great, but I'd argue "UNKNOWN ERROR" is better than literally nothing

Does this introduce a breaking change?

  • Yes
  • No

Testing

Unit tests were run with coverage to validate we handle the entirety of this method

Other information

Please do not merge, relies on #3211

@rwaskiewicz rwaskiewicz changed the title Add testing to message utils test(message-utils): add message-utils testing Jan 21, 2022
@rwaskiewicz rwaskiewicz marked this pull request as ready for review January 24, 2022 18:49
@rwaskiewicz rwaskiewicz requested a review from a team January 24, 2022 18:49
} else if (err != null) {
if (err.stack != null) {
diagnostic.messageText = err.stack.toString();
} else {
if (err.message != null) {
diagnostic.messageText = err.message.toString();
diagnostic.messageText = err.message.length ? err.message.toString() : 'UNKNOWN ERROR';
Copy link
Member

Choose a reason for hiding this comment

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

Presumably it's safe to remove toString() since err.message is a string?

Suggested change
diagnostic.messageText = err.message.length ? err.message.toString() : 'UNKNOWN ERROR';
diagnostic.messageText = err.message.length ? err.message : 'UNKNOWN ERROR';

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, removing in 3fa068d..e131cca

Base automatically changed from useUnknownInCatchVars to main January 27, 2022 19:42
add testing to src/util/message-utils.ts
@rwaskiewicz rwaskiewicz merged commit d1d34cc into main Jan 27, 2022
@rwaskiewicz rwaskiewicz deleted the add-testing-to-message-utils branch January 27, 2022 20:13
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.

2 participants