Skip to content

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

Merged
rwaskiewicz merged 1 commit intomainfrom
add-testing-to-message-utils
Jan 27, 2022
Merged

test(message-utils): add message-utils testing#3212
rwaskiewicz merged 1 commit intomainfrom
add-testing-to-message-utils

Conversation

@rwaskiewicz
Copy link
Copy Markdown
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.message != null) {
diagnostic.messageText = err.message.toString();
diagnostic.messageText = err.message.length ? err.message.toString() : 'UNKNOWN ERROR';
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

@rwaskiewicz rwaskiewicz force-pushed the useUnknownInCatchVars branch from b88604b to f223d61 Compare January 27, 2022 18:51
Base automatically changed from useUnknownInCatchVars to main January 27, 2022 19:42
add testing to src/util/message-utils.ts
@rwaskiewicz rwaskiewicz force-pushed the add-testing-to-message-utils branch from e131cca to a05edec Compare January 27, 2022 19:45
@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