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

Asserting a range with descriptive error messages #4461

Open
nicholaswmin opened this issue Aug 3, 2024 · 15 comments
Open

Asserting a range with descriptive error messages #4461

nicholaswmin opened this issue Aug 3, 2024 · 15 comments

Comments

@nicholaswmin
Copy link

nicholaswmin commented Aug 3, 2024

Node.js Version

v22

NPM Version

10.8.2

Operating System

MacOS Sonoma

Subsystem

assert

Description

I work a lot with performance testing code which frequently leads in the unfortunate position where I need to assert ranges rather than exact values, i.e

is value between 1500 and 2000?

Now, to do that I use a pair of assert.ok like so:

const value = 1700

assert.ok(value >= 1500)
assert.ok(value <= 2000)

so far so good, could be better but that's clean and concise enough for my taste.

The problem is that setting a descriptive error message quickly becomes rather repetitive and convoluted.

I have to resort to something akin to this:

assert.ok(value >= 1500, `expected value >= 1500 , got: ${value}`)
assert.ok(value <= 2000, `expected value <= 2000 , got: ${value}`)

Apart from cooking up my own assertion functions, is there a better way to do this out-the-box that I'm missing?

@RedYetiDev
Copy link
Member

CC @nodejs/assert

@gireeshpunathil
Copy link
Member

looks reasonable to me!

@gireeshpunathil
Copy link
Member

@nicholaswmin - pls feel free to raise a PR if you are willing to!

@RedYetiDev
Copy link
Member

Although, doesn't the assert message include the failed assertion line, so wouldn't it already show the failure?

@nicholaswmin
Copy link
Author

nicholaswmin commented Sep 4, 2024

@RedYetiDev here's the output:

import test from 'node:test'

await test('is within range',  async t => {
  const value = 2700
  
  t.assert.ok(value >= 1500)
  t.assert.ok(value <= 2000)
})
✖ is within range (1.220792ms)
  AssertionError [ERR_ASSERTION]: false == true
      at assert.<computed> [as ok] (node:internal/test_runner/test:261:18)
      at TestContext.<anonymous> (file:///Users/nicholaswmin/Projects/fsm/scratch.js:7:12)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:878:25)
      at Test.processPendingSubtests (node:internal/test_runner/test:590:18)
      at node:internal/test_runner/harness:252:20
      at node:internal/process/task_queues:151:7
      at AsyncResource.runInAsyncScope (node:async_hooks:211:14)
      at AsyncResource.runMicrotask (node:internal/process/task_queues:148:8) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: false,
    expected: true,
    operator: '=='
  }

My "complaint" is that it doesn't print value being asserted against, unless I specifically code it to so with that string interpolation.

Now granted, the above is impossible to do with the code I'm providing because that's a Boolean expression.
An ideal API might be something like:

  t.assert.ok(value <= 2000, value)
  t.assert.ok(value <= 2000, value)

AssertionError [ERR_ASSERTION]: Failed, value: 2700 was not ok

Hmm.. I'm a bit confused... Not sure why I opened this issue as what I'm proposing sounds like a silly API.

Doing this:

t.assert.ok(value <= 2000, value)

prints the value anyway.

I don't remember what I had in mind. Sorry.

@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 4, 2024

assert.isGreaterThan (and vise versa) could work if you'd like to add it?

@nicholaswmin
Copy link
Author

nicholaswmin commented Sep 4, 2024

I'm personally against adding specific assertion checks to assert like that.

It'll eventually snowball into a myriad utilities. I miss the hyper-declarative syntax of Chai but I've also come to love the minimal, no-frills API surface of assert since I don't need to memorise a million method names.

... I don't remember what i had in mind when i opened this.

If I need range checks I can extend assert locally.

I think this needs to be closed ??

@nicholaswmin
Copy link
Author

nicholaswmin commented Sep 4, 2024

The closest thing I can figure out that would help without adding specific assertion checks is printing the expression itself on assert.ok().

An example:

import test from 'node:test'

await test('foo', async t => {
  const value = 2700
  
  t.assert.ok(value >= 1500)
  t.assert.ok(value <= 2000)
})

logs:

✖ is in range (1.220875ms)
  AssertionError [ERR_ASSERTION]: `value >= 1500`
      at assert.<computed> [as ok] (node:internal/test_runner/test:261:18)
      at TestContext.<anonymous> (file:///Users/nicholaswmin/Projects/fsm/scratch.js:7:12)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:878:25)
      at Test.processPendingSubtests (node:internal/test_runner/test:590:18)
      at node:internal/test_runner/harness:252:20
      at node:internal/process/task_queues:151:7
      at AsyncResource.runInAsyncScope (node:async_hooks:211:14)
      at AsyncResource.runMicrotask (node:internal/process/task_queues:148:8) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: false,
    expected: true,
    operator: '=='
  }

which is more descriptive than the current:

AssertionError [ERR_ASSERTION]: false == true

Not sure if that's doable without doing esprima-like parsing though?

@RedYetiDev
Copy link
Member

For your specific case, a custom error (E.G. throw new assert.AssertionError({actual: 1, expected:2, operator:">="})) can be thrown.

Not sure if that's doable without doing esprima-like parsing though?

In some cases, that is what happens:

import assert from 'node:assert';

assert.ok(25 > 50);
node:internal/modules/run_main:123
    triggerUncaughtException(
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(25 > 50)

    at file:///Users/avivkeller/Documents/tmp/repro.mjs:3:8
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Node.js v22.8.0

@nicholaswmin
Copy link
Author

nicholaswmin commented Sep 4, 2024

In some cases, that is what happens:

Yes. That works! That's more than enough.

I can repro your working example with assert but not with t.assert

@RedYetiDev
Copy link
Member

I'm aware. I'm opening an issue for it as we speak.

@nicholaswmin
Copy link
Author

want me to have a look into it?

@RedYetiDev
Copy link
Member

If you'd like, see nodejs/node#54760

@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 4, 2024

Fixed in nodejs/node#54776

@nicholaswmin
Copy link
Author

nicholaswmin commented Sep 4, 2024

Amazing;

sorry, life got in the way ...

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Sep 15, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Sep 16, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Sep 16, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Sep 17, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this issue Sep 17, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Sep 22, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Sep 26, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Oct 2, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Oct 2, 2024
PR-URL: #54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
PR-URL: nodejs#54776
Refs: nodejs/help#4461
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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

No branches or pull requests

3 participants