Skip to content

assert.js rule 7.4 (similar to CJS rule 7.3) #9063

Closed
Closed
assert.js rule 7.4 (similar to CJS rule 7.3)#9063
Labels
assertIssues and PRs related to the assert subsystem.good first issueIssues that are suitable for first-time contributors.

Description

@mk-pmb
mk-pmb
opened on Oct 12, 2016
  • Version: v6.1.0
  • Platform: Linux ███ 3.13.0-97-generic #144-Ubuntu SMP Thu Sep 22 16:23:22 UTC 2016 i686 i686 i686 GNU/Linux
  • Subsystem: assert

Hi,

I'm developing an assert lib and it disagrees with node's. While debugging, I found rule 7.4 in master/lib/assert.js @ d4061a6 that to me sounds very similar to rule 7.3 in the CJS spec:

  // 7.4. Other pairs that do not both pass typeof value == 'object',
  // equivalence is determined by ==.
  } else if ((actual === null || typeof actual !== 'object') &&
    (expected === null || typeof expected !== 'object')) {

I think the condition tests both values for being primitives, aka not objects. Firefox seems to agree:

var actual = true, expected = [ 1 ];
console.log([ (actual === null || typeof actual !== 'object'), '&&',
    (expected === null || typeof expected !== 'object') ]);
// -> Array [ true, "&&", false ]

So one passes the object check, the other does not, as expected.
Is it a pair where both values pass the object check? No.
So shouldn't it count as a "[pair] that do not both pass typeof […] object"?
If the partial conditions negate the object check and check for being primiteves instead, shouldn't the binary operator change to ||?

Pre-emptive: In case anyone feels inclined to discuss the quality of the CJS UT spec and/or how much better deepStrictEqual is, please make another issue for that.

Activity

  1. cjihrig commented on Oct 12, 2016

    @cjihrig
    cjihrig
    on Oct 12, 2016
    Contributor

    Just to clarify, can you provide a call to the assert method that is working incorrectly?

  2. mk-pmb commented on Oct 12, 2016

    @mk-pmb
    mk-pmb
    on Oct 12, 2016
    Author

    Nope, that would be the next step. I first have to understand exactly which behavior is intended as correct; only then I can know whether it is working incorrectly at all.

    The code example in the other thread is not applicable because that one is about the CJS UT spec, the one at the URL stated in line 1 of node's assert.js.
    However, that's obviously not the spec being used here, as it has no mention of RegExps at all, whereas node's assert.js has rule 7.3 specifically for RegExps.
    (I don't care why the URL is in line 1 – if anyone does, please make another issue for that. ;-) )

    We can care about specs later, but for now my goal in this issue is just to solve the cited dissonance between comment and code.

  3. mscdex
    added
    assertIssues and PRs related to the assert subsystem.
    on Oct 12, 2016

  4. Trott commented on Oct 12, 2016

    @Trott
    Trott
    on Oct 12, 2016
    Member

    If it helps explain anything: The 7.4 and some of the other numbers in the comments are just wrong. They were changed in a805012d and almost certainly in other commits as well. Apparently people did not realize that the numbers (of the form 7.3) were not arbitrarily invented by Node.js but were part of an external standard, so they just modified them when it seemed to make sense to them. Thus, they are out of sync with the standard.

    Side note: A good first commit for a new contributor might be to remove all those numbers so that no one thinks they are supposed to map to the standard. We've likely deviated from it and even if we haven't, we likely happily would if it suited our needs.

  5. mk-pmb commented on Oct 12, 2016

    @mk-pmb
    mk-pmb
    on Oct 12, 2016
    Author

    Removing the old numbers, and adding an annotation to the spec URL (or removing it) will probably avoid some bug reports in case the code is kept and the comment is edited to match it.
    I suggest to add new numbers though (maybe prefixed with "N"?) to simplify discussion about them independent of line numbers.

  6. Fishrock123 commented on Oct 12, 2016

    @Fishrock123
    Fishrock123
    on Oct 12, 2016
    Contributor

    We don't follow CJS so the comments should not be considered part or forming any standard but rather just regular code comments. They may change at any time.

  7. mk-pmb commented on Oct 13, 2016

    @mk-pmb
    mk-pmb
    on Oct 13, 2016
    Author

    How about replacing the comment that was 7.4 with:

    If both values are primitives, compare them by (optionally strict) equality.

    I think that's what the current code does.

  8. Trott commented on Oct 13, 2016

    @Trott
    Trott
    on Oct 13, 2016
    Member

    A contribution that makes the comments more accurately and clearly reflect what is going on in the code would be welcome.

  9. Trott
    added
    good first issueIssues that are suitable for first-time contributors.
    on Oct 13, 2016
  10. jasnell
    closed this as completedin 398229aon Jan 4, 2017
  11. italoacasas
    added 4 commits that reference this issue on Jan 18, 2017

    assert: update comments

    6030faf

    assert: update comments

    4edd3c2

    assert: update comments

    480aad8

    assert: update comments

    b2d0c44
  12. MylesBorins
    added 4 commits that reference this issue on Mar 7, 2017

    assert: update comments

    33a3861

    assert: update comments

    075de21

    assert: update comments

    7ecfe49

    assert: update comments

    994f562
  13. sjarmak
    added a commit that references this issue on Jul 27, 2026

    assert: update comments

    398229a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    assertIssues and PRs related to the assert subsystem.good first issueIssues that are suitable for first-time contributors.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions