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] Add tests for async progress queue worker #1316

Merged
merged 2 commits into from
Jun 10, 2023

Conversation

JckXia
Copy link
Member

@JckXia JckXia commented May 5, 2023

Resolves #1043.

Similar to PR #1307 but for AsyncProgressQueueWorker

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

Looks good! This covers the remaining untested methods listed in #1043.

Just a couple of small changes. Take a look at my comments (they are applicable to multiple locations in the code as well).

Guess I didn't catch these in #1307 ... whoops 😅

}

async function asyncProgressWorkerNoCbOverloads (bindingFunction) {
bindingFunction(common.mustCall(() => {}));
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for noop arg for mustCall.

Comment on lines 52 to 53
}).catch(common.mustNotCall());
resolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

The common.mustNotCall() should be the Promise's reject callback, and the resolve() belongs inside the hooks.then() callback after the assertion.

This will provide the actual assertion error (in case of failure). Try changing the assertion object, eg. the destroy event name to __destroy.

With your code, you'll get:

> AssertionError [ERR_ASSERTION]: function should not have been called
    at mustNotCall (/Users/kevineady/Documents/Projects/node-addon-api/test/common/index.js:120:12) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: undefined,
  operator: 'fail'
}

With the suggested changes, you'll get:

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

  [
    {
...
    },
    {
+     eventName: 'destroy'
-     eventName: '__destroy'
    }
  ]
    at /Users/kevineady/Documents/Projects/node-addon-api/test/async_progress_queue_worker.js:41:14 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: [Array],
  expected: [Array],
  operator: 'deepStrictEqual'
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the wait. Thank you for pointing this out! I borrowed this pattern from other places in the test suites. Will draft up another PR to make these fixes.

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

LGTM !

@JckXia JckXia merged commit a198e24 into nodejs:main Jun 10, 2023
27 checks passed
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.

[Tests] AsyncProgressQueueWorker unit test coverage
2 participants