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: use regex for OpenSSL function name #28289

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Jun 19, 2019

This commit modifies test-crypt-scrypt.js to use a regular expression
for the function name in the error message, similar to what is done for
the error code.

The motivation for this change comes from a case where we (Red Hat)
patch OpenSSL and the memory limit checking is done in a different
function, meaning that the function name from which this error
originates differs from that when linking to the OpenSSL version shipped
with Node.js.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This commit modifies test-crypt-scrypt.js to use a regular expression
for the function name in the error message, similar to what is done for
the error code.

The motivation for this change comes from a case where we (Red Hat)
patch OpenSSL and the memory limit checking is done in a different
function, meaning that the function name from which this error
originates differs from that when linking to the OpenSSL version shipped
with Node.js.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 19, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

I'm OK with this as-is, but wonder if might be better to just switch the test to assert on the .code instead of the .message, which should be more stable going forward, even openssl might move the check around to a different code location.

@danbev
Copy link
Contributor Author

danbev commented Jun 20, 2019

I'm OK with this as-is, but wonder if might be better to just switch the test to assert on the .code

I like that, I'll make the changes. Thanks!

@danbev
Copy link
Contributor Author

danbev commented Jun 20, 2019

@sam-github Before I start I just wanted to mention that there currently is no .code property for this error, only a .message. Looking at the comment in the code it says:

// EVP_PBE_scrypt() does not always put errors on the error stack
// and therefore ToResult() may or may not return an exception
// object.  Return a sentinel value to inform JS land it should
// throw an ERR_CRYPTO_SCRYPT_PARAMETER_ERROR on our behalf.

But in the memory limit exceeded case there is indeed an exception which means that the exception will be thrown as is.

I'm happy to look into a solution but wanted to make sure this is what we actually want to do here and that the lack of .code property was taking into consideration (from looking at other parts of that test it would seem like there is a .code property but not in all cases).

test/parallel/test-crypto-scrypt.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

There aren't .code for everything, its unfortunate. It keeps getting better. So, a regex is probably best for this specific PR. Anything you can do to improve the situation is great, but likely the easy errors already have codes :-).

@danbev
Copy link
Contributor Author

danbev commented Jun 24, 2019

Re-build of failing node-test-commit-aix (✔️)

@danbev danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 24, 2019
@danbev
Copy link
Contributor Author

danbev commented Jun 24, 2019

Landed in c1ee668.

@danbev danbev closed this Jun 24, 2019
@danbev danbev deleted the scrypt-mem-limit-exceeded-test branch June 24, 2019 05:31
danbev added a commit that referenced this pull request Jun 24, 2019
This commit modifies test-crypt-scrypt.js to use a regular expression
for the function name in the error message, similar to what is done for
the error code.

The motivation for this change comes from a case where we (Red Hat)
patch OpenSSL and the memory limit checking is done in a different
function, meaning that the function name from which this error
originates differs from that when linking to the OpenSSL version shipped
with Node.js.

PR-URL: #28289
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
This commit modifies test-crypt-scrypt.js to use a regular expression
for the function name in the error message, similar to what is done for
the error code.

The motivation for this change comes from a case where we (Red Hat)
patch OpenSSL and the memory limit checking is done in a different
function, meaning that the function name from which this error
originates differs from that when linking to the OpenSSL version shipped
with Node.js.

PR-URL: #28289
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@targos targos mentioned this pull request Jul 2, 2019
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants