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

assert: accept regular expressions to validate #20485

Closed
wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 2, 2018

This makes sure regular expressions on validation objects validate
against strings when used with assert.throws and assert.rejects.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This makes sure regular expressions on validation objects validate
against strings when used with `assert.throws` and `assert.rejects`.
@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label May 2, 2018
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 2, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented May 2, 2018

an object where each property will be tested for, or an instance of error where
each property will be tested for including the non-enumerable `message` and
`name` properties.
an validation object where each property will be tested for strict deep
Copy link
Contributor

Choose a reason for hiding this comment

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

an validation -> a validation.

an validation object where each property will be tested for strict deep
equality, or an instance of error where each property will be tested for strict
deep equality including the non-enumerable `message` and `name` properties. When
using an validation object, it is also possible to use a regular expression,
Copy link
Contributor

Choose a reason for hiding this comment

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

an validation -> a validation.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented May 3, 2018

I am a bit confused.

When using an validation object, it is also possible to use a regular expression, when validating against a string property.

From this description, I would infer that a RegExp from validation object would be checked against a string value of the same property in the thrown error.

But in the code and comments we have two RegExps:

err.reg = /abc/i;

// ...

    // This will strictly validate that the regular expression is identical to
    // the regular expression on the error.
    reg: /abc/i

How does this relate?

@BridgeAR
Copy link
Member Author

BridgeAR commented May 3, 2018

@vsemozhetbyt they are both checked. That is the key point. It is unlikely that regular expressions are going to exist on errors but if they do, they are still validated properly.

@vsemozhetbyt
Copy link
Contributor

Then maybe we need both examples in the code example? RegExp against RegExp and RegExp against the string, both pairs for the same name properties?

@BridgeAR
Copy link
Member Author

BridgeAR commented May 3, 2018

@vsemozhetbyt that is exactly my test case. So I guess it is not obvious enough so far. I tried to make it clear, but I am going to iterate over it later on.

a validation object where each property will be tested for strict deep equality,
or an instance of error where each property will be tested for strict deep
equality including the non-enumerable `message` and `name` properties. When
using a object, it is also possible to use a regular expression, when validating
Copy link
Contributor

Choose a reason for hiding this comment

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

an object)

@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018

Comments addressed.

New CI https://ci.nodejs.org/job/node-test-pull-request/14699/

@nodejs/testing PTAL

@BridgeAR
Copy link
Member Author

BridgeAR commented May 9, 2018

PTAL

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2018
@BridgeAR BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label May 10, 2018
@BridgeAR
Copy link
Member Author

Landed in cf7be86

@BridgeAR BridgeAR closed this May 10, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 10, 2018
This makes sure regular expressions on validation objects validate
against strings when used with `assert.throws` and `assert.rejects`.

PR-URL: nodejs#20485
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request May 12, 2018
This makes sure regular expressions on validation objects validate
against strings when used with `assert.throws` and `assert.rejects`.

PR-URL: #20485
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit to addaleax/node that referenced this pull request May 14, 2018
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
@BeyondEvil
Copy link

Can we get this in Assert.rejects as well?

@BridgeAR
Copy link
Member Author

BridgeAR commented Nov 5, 2019

@BeyondEvil it should work identically using .rejects. Please just give it a try or post an example that fails.

@BeyondEvil
Copy link

BeyondEvil commented Nov 5, 2019

@BeyondEvil it should work identically using .rejects. Please just give it a try or post an example that fails.

Thanks @BridgeAR , I did give it a try. Searching for the solution led me here. 😊

Maybe I'm doing something wrong, but here's what I tried:

	it('should fail if incorrect url', async () => {
		const domain = 'sekhfskdfdjksfkjdhfsd.com'
		const incorrectUrl = `https://${domain}`
		await assert.rejects(
			post(incorrectUrl),
			new SemanticReleaseError(/.+ENOTFOUND.+/, 'SLACK CONNECTION FAILED')
		)
	})

result:

  1) test postMessage
       should fail if incorrect url:

      AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual

  Comparison {
    code: 'SLACK CONNECTION FAILED',
    details: undefined,
-   message: 'request to https://sekhfskdfdjksfkjdhfsd.com/ failed, reason: getaddrinfo ENOTFOUND sekhfskdfdjksfkjdhfsd.com sekhfskdfdjksfkjdhfsd.com:443',
+   message: '/.+ENOTFOUND.+/',
    name: 'SemanticReleaseError',
    semanticRelease: true
  }
      + expected - actual

SemanticReleaseError

What am I doing wrong?

@BridgeAR
Copy link
Member Author

BridgeAR commented Nov 5, 2019

You pass through a regular expression to your error constructor. That is first coerced to a string and then set as message. During the assertion the message is now checked against the errors message property which is a string and the comparison fails.

I guess you want to do something like:

assert.rejects(
  post(foobar),
  {
    name: 'SemanticReleaseError',
    code: 'SLACK CONNECTION FAILED',
    details: undefined,
    message: /ENOTFOUND/,
    semanticRelease: true
  }
)

@BeyondEvil
Copy link

Ah, of course! Thank you, much appreciated! 🙏

@BridgeAR BridgeAR deleted the assert-add-reg-exp branch January 20, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants