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: fix 'checks' validation test for checkPrime #47139

Merged

Conversation

tniessen
Copy link
Member

This test had two problems:

  • The first argument was a number in both cases, which is what caused the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the checks option was not actually verified at all. Thus, the first argument should be valid for this particular test.
  • The function returned by common.mustNotCall() was passed to assert.throws() as a third argument instead of being passed to checkPrime() as a third argument. (Isn't JavaScript great?) This again led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the validity of the checks option was not verified.

Fix both issues by ensuring that all arguments except the checks option are valid.

Refs: #36997

This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: nodejs#36997
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Mar 17, 2023
@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. request-ci Add this label to start a Jenkins CI on a PR. needs-ci PRs that need a full CI run. and removed needs-ci PRs that need a full CI run. labels Mar 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 17, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2023
@tniessen tniessen added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 17, 2023
@nodejs nodejs deleted a comment from github-actions bot Mar 18, 2023
@tniessen tniessen added commit-queue Add this label to land a pull request using GitHub Actions. and removed fast-track PRs that do not need to wait for 48 hours to land. labels Mar 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 19, 2023
@nodejs-github-bot nodejs-github-bot merged commit fbd526b into nodejs:main Mar 19, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in fbd526b

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: #36997
PR-URL: #47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: #36997
PR-URL: #47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
This test had two problems:

* The first argument was a number in both cases, which is what caused
  the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
  'checks' option was not actually verified at all. Thus, the first
  argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
  assert.throws() as a third argument instead of being passed to
  checkPrime() as a third argument. (Isn't JavaScript great?) This again
  led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
  validity of the 'checks' option was not verified.

Fix both issues by ensuring that all arguments except the 'checks'
option are valid.

Refs: #36997
PR-URL: #47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants