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(expect): support expect.hasAssertions() #5901

Merged
merged 14 commits into from
Sep 19, 2024

Conversation

eryue0220
Copy link
Contributor

This pr is one of part to solve #3964, which is to support expect.hasAssertion api.

@eryue0220 eryue0220 requested a review from kt3k as a code owner September 4, 2024 03:21
@eryue0220 eryue0220 changed the title feat(std/expect): support expect.hasAssertion api feat(expect/unstable): support expect.hasAssertion api Sep 4, 2024
@eryue0220 eryue0220 marked this pull request as draft September 4, 2024 03:31
@iuioiua iuioiua changed the title feat(expect/unstable): support expect.hasAssertion api feat(expect/unstable): support expect.hasAssertion() Sep 4, 2024
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 89.06250% with 7 lines in your changes missing coverage. Please review.

Project coverage is 96.30%. Comparing base (1d22380) to head (1d55626).
Report is 61 commits behind head on main.

Files with missing lines Patch % Lines
testing/bdd.ts 42.85% 4 Missing ⚠️
expect/_assertion.ts 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5901      +/-   ##
==========================================
- Coverage   96.31%   96.30%   -0.01%     
==========================================
  Files         482      495      +13     
  Lines       39388    39571     +183     
  Branches     5824     5839      +15     
==========================================
+ Hits        37935    38108     +173     
- Misses       1411     1421      +10     
  Partials       42       42              

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This file doesn't seem needed. We should just simply throw AssertionError in testing/bdd.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@eryue0220 eryue0220 marked this pull request as ready for review September 4, 2024 04:44

const assertionState = getAssertionState();

export function hasAssertion() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export function hasAssertion() {
export function hasAssertions() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@iuioiua iuioiua requested a review from kt3k September 9, 2024 03:41
@eryue0220 eryue0220 marked this pull request as draft September 9, 2024 04:02
@kt3k kt3k changed the title feat(expect/unstable): support expect.hasAssertion() feat(expect/unstable): support expect.hasAssertions() Sep 9, 2024
testing/bdd.ts Outdated Show resolved Hide resolved
@kt3k kt3k changed the title feat(expect/unstable): support expect.hasAssertions() fix(expect/unstable): support expect.hasAssertions() Sep 9, 2024
expect/expect.ts Outdated Show resolved Hide resolved
@kt3k kt3k changed the title fix(expect/unstable): support expect.hasAssertions() feat(expect/unstable): support expect.hasAssertions() Sep 9, 2024
@kt3k
Copy link
Member

kt3k commented Sep 9, 2024

This seems only works for the first use of hasAssertions as we don't create a new assertionState for each it call.

The below script doesn't throw with the 2nd case, which misses assertions:

import { expect } from "@std/expect";
import { test } from "@std/testing/bdd";

test("1", () => {
  expect.hasAssertions();
  expect(1).toBe(1);
});
test("2", () => {
  expect.hasAssertions();
});

@eryue0220 eryue0220 marked this pull request as ready for review September 10, 2024 16:37
@eryue0220
Copy link
Contributor Author

This seems only works for the first use of hasAssertions as we don't create a new assertionState for each it call.

The below script doesn't throw with the 2nd case, which misses assertions:

import { expect } from "@std/expect";
import { test } from "@std/testing/bdd";

test("1", () => {
  expect.hasAssertions();
  expect(1).toBe(1);
});
test("2", () => {
  expect.hasAssertions();
});

Yes, the above code had some bugs. and I've updated.

Thanks.

@@ -383,5 +389,11 @@ export class TestSuiteInternal<T> implements TestSuite<T> {
} else {
await fn.call(context, t);
}

if (assertionState.checkAssertionErrorStateAndReset()) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel this check is unnecessary. If I removed this check, the test cases still pass.

I'm removing this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a little tricky about this, I think you run the test inside Deno.test and then it will never throw error. But if you remove it, then it works.

@kt3k kt3k changed the title feat(expect/unstable): support expect.hasAssertions() fix(expect): support expect.hasAssertions() Sep 13, 2024
@kt3k
Copy link
Member

kt3k commented Sep 13, 2024

This API has been documented as 'still not available'. I think we can consider this addition as 'fix' as this is already expected to be added.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for updating!

@kt3k kt3k merged commit 6a4eb6c into denoland:main Sep 19, 2024
17 checks passed
@eryue0220 eryue0220 deleted the feat/expect-assertions-api branch September 19, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants