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

feat(assert): add options parameter to AssertionError constructor #5561

Merged
merged 7 commits into from
Jul 29, 2024
Merged

feat(assert): add options parameter to AssertionError constructor #5561

merged 7 commits into from
Jul 29, 2024

Conversation

jsejcksn
Copy link
Contributor

@jsejcksn jsejcksn requested a review from kt3k as a code owner July 28, 2024 09:59
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.48%. Comparing base (72e2462) to head (554ba09).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5561   +/-   ##
=======================================
  Coverage   96.48%   96.48%           
=======================================
  Files         465      465           
  Lines       37772    37772           
  Branches     5580     5580           
=======================================
  Hits        36445    36445           
  Misses       1285     1285           
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Are you able to add a test the ensures that the cause correctly propagates? Also, can you please add an example showing this behavior?

@jsejcksn
Copy link
Contributor Author

@iuioiua

Are you able to add a test the ensures that the cause correctly propagates?

I didn't see a test module for this class, but I'm happy to create one. I'll add an equality test for the error message property parameter as well.

Also, can you please add an example showing this behavior?

I'm not sure what you have in mind. Do you essentially want me to copy the body of the test? If not, can you make a suggestion?

@jsejcksn
Copy link
Contributor Author

@iuioiua I added tests using actual control flow. As a contrived but simpler alternative, it could look like this:

Deno.test("AssertionError", () => {
  const originalError = new Error("foo");
  const assertionError = new AssertionError("bar", { cause: originalError });
  assertStrictEquals(assertionError.message, "bar");
  assertStrictEquals(assertionError.cause, originalError);
});

…but that doesn't represent intended usage in exception-handling control flow. With that juxtaposition available, what are your current thoughts about the following?

  1. the structure/style of the tests
  2. the content of the example documentation

@jsejcksn
Copy link
Contributor Author

Aside: I'm seeing ci failures in the lint steps (example). They are for the rule no-unreachable, and I think they shouldn't be happening (the tests prove that the lint diagnostic is wrong). Perhaps a new issue should be raised in that repository…

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

I've cleaned up the example and test (they don't have to be too thorough, as this error is used to a large extend throughout testing in the codebase). Now, LGTM! Thanks!

@iuioiua iuioiua enabled auto-merge (squash) July 29, 2024 00:48
auto-merge was automatically disabled July 29, 2024 02:07

Head branch was pushed to by a user without write access

@jsejcksn
Copy link
Contributor Author

they don't have to be too thorough

@iuioiua Ok, sounds good. I did make one more change: using strict equality comparison for the error.cause because I think it's important to illustrate that they aren't just the same shapes, but actually the same value.

Do you have any thoughts about #5561 (comment)?

@iuioiua
Copy link
Contributor

iuioiua commented Jul 29, 2024

29bb954

Yes please.

@jsejcksn
Copy link
Contributor Author

29bb954

Yes please.

Ref:

@iuioiua iuioiua enabled auto-merge (squash) July 29, 2024 03:59
@iuioiua iuioiua disabled auto-merge July 29, 2024 04:00
@iuioiua iuioiua merged commit 36bc450 into denoland:main Jul 29, 2024
13 checks passed
@jsejcksn jsejcksn deleted the feat/assert/assertion-error-options branch July 29, 2024 08:17
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.

2 participants