docs: improve assert docs by BridgeAR · Pull Request #20313 · nodejs/node

BridgeAR · 2018-04-26T01:10:26Z

This improves the error example output by reflecting the current
state. It also makes sure the examples are up to date in general.
assert.throws clarified the ERR_AMBIGUOUS_ARGUMENT error.

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 improves the error example output by reflecting the current state. It also makes sure the examples are up to date in general. `assert.throws` clarified the `ERR_AMBIGUOUS_ARGUMENT` error.

BridgeAR · 2018-04-26T01:11:02Z

doc/api/assert.md

 assert.deepStrictEqual({ a: 1 }, { a: '1' });
-// AssertionError: { a: 1 } deepStrictEqual { a: '1' }
-// because 1 !== '1' using SameValue comparison
+// AssertionError: Input A expected to strictly deep-equal input B:


I removed the ERR_ASSERTION part in some cases because the line length would otherwise be above 80 characters.

BridgeAR · 2018-04-26T01:26:42Z

CI https://ci.nodejs.org/job/node-test-pull-request-lite/596/

vsemozhetbyt · 2018-04-26T02:05:15Z

I have some deviations from the the very first example or even strange issues on Windows 7 x64 (Node.js v10.0.0) concerning overall color handling and line breaks in the legend:

Outputs + screenshots for 4 cases with the same code:

cmd.exe, REPL;

> require('assert').strict.deepEqual([1, 2, 3, 4, 5], [1, 2, 3, 4, '5']);
AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual ... Lines skipped

  [
    1,
...
    4,
-   5
+   '5'
  ]
>

cmd.exe, script;

> node.10.0.0.v8-6.6.exe e:\DOC\prg\js\node\-test\test.js
assert.js:77
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
 [32m+ expected [39m  [31m- actual [39m ... Lines skipped

  [
    1,
...
    4,
 [31m- [39m   5
 [32m+ [39m   '5'
  ]
    at Object.<anonymous> (e:\DOC\prg\js\node\-test\test.js:3:26)
    at Module._compile (internal/modules/cjs/loader.js:678:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
    at Module.load (internal/modules/cjs/loader.js:589:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:528:12)
    at Function.Module._load (internal/modules/cjs/loader.js:520:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)
    at startup (internal/bootstrap/node.js:228:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)

git-bash.exe, REPL;

> require('assert').strict.deepEqual([1, 2, 3, 4, 5], [1, 2, 3, 4, '5']);
AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual ... Lines skipped

  [
    1,
...
    4,
-   5
+   '5'
  ]
>

git-bash.exe, script;

winpty ./node.10.0.0.v8-6.6.exe e:/DOC/prg/js/node/-test/test.js
assert.js:77
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
[32m+ expected[39m [31m- actual[39m ... Lines skipped

  [
    1,
...
    4,
[31m-[39m   5
[32m+[39m   '5'
  ]
    at Object.<anonymous> (e:\DOC\prg\js\node\-test\test.js:3:26)
    at Module._compile (internal/modules/cjs/loader.js:678:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
    at Module.load (internal/modules/cjs/loader.js:589:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:528:12)
    at Function.Module._load (internal/modules/cjs/loader.js:520:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)
    at startup (internal/bootstrap/node.js:228:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:575:3)

Should we ignore this?

BridgeAR · 2018-04-26T02:18:37Z

@vsemozhetbyt I removed the extra line break in here to make it clear to the reader that it is still the same error message. Besides that I do not see any differences concerning the line breaks. The color handling is indeed weird though and that is a separate concern from this PR. I am going to try to look into it, but I sadly do not have a Windows to debug this at the moment.

BridgeAR · 2018-04-26T02:26:35Z

@vsemozhetbyt oh, I cite: Example error diff (the expected, actual, and Lines skipped will be on a single row):. I did that to make sure the - and + entries are colored when looked at as assert.md. I guess that could be changed as it is mainly about the website anyway.

vsemozhetbyt · 2018-04-26T02:32:08Z

BTW, they are not colored in the doc site: https://nodejs.org/download/nightly/v11.0.0-nightly2018042595197ed2b0/docs/api/assert.html#assert_strict_mode

BridgeAR · 2018-04-26T02:47:12Z

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

vsemozhetbyt

With some possible nits)

vsemozhetbyt · 2018-04-26T02:44:51Z

doc/api/assert.md

 assert.deepStrictEqual(weakMap1, weakMap2);
 // OK, because it is impossible to compare the entries
+
+// Fail because weakMap3 has a property that weakMap1 does not contain:


Fail -> Fails? (Ignore if this is a noun)

vsemozhetbyt · 2018-04-26T02:50:29Z

doc/api/assert.md

 Throws `value` if `value` is not `undefined` or `null`. This is useful when
-testing the `error` argument in callbacks.
+testing the `error` argument in callbacks. The stack trace contains all frames
+from the error passed to `ifError` including the potential new frames for


`ifError` -> `ifError()`?

vsemozhetbyt · 2018-04-26T02:50:36Z

doc/api/assert.md

-testing the `error` argument in callbacks.
+testing the `error` argument in callbacks. The stack trace contains all frames
+from the error passed to `ifError` including the potential new frames for
+`ifError` itself. See below for an example.


`ifError` -> `ifError()`?

vsemozhetbyt · 2018-04-26T02:57:56Z

doc/api/assert.md

 assert.ok();
-// throws:
-// "AssertionError: No value argument passed to `assert.ok`.
+// AssertionError: No value argument passed to `assert.ok`.


`assert.ok()` ?

TODO for another PR: unify parentheses in notStrictEqual above and check other cases?

Trott

LGTM with nits from @vsemozhetbyt addressed

BridgeAR · 2018-04-26T11:14:51Z

Comments addressed.

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

trivikr · 2018-04-26T15:55:07Z

doc/api/assert.md


 assert.ok(false, 'it\'s false');
-// throws "AssertionError: it's false"
+// AssertionError: it's false"


nit: remove "

Trott · 2018-04-26T21:43:27Z

doc/api/assert.md


 assert.ok();
-// AssertionError: No value argument passed to `assert.ok`.
+// AssertionError: No value argument passed to `assert.ok()`.


FWIW, there's no period at the end of the error message:

> assert.ok() AssertionError [ERR_ASSERTION]: No value argument passed to `assert.ok()` >

BridgeAR · 2018-04-27T12:45:29Z

Comments addressed. I also moved an example higher up, so it is more prominent.

CI https://ci.nodejs.org/job/node-test-pull-request-lite/607/

BridgeAR · 2018-04-29T15:09:43Z

Landed in 6554301

This improves the error example output by reflecting the current state. It also makes sure the examples are up to date in general. `assert.throws` clarified the `ERR_AMBIGUOUS_ARGUMENT` error. PR-URL: nodejs#20313 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

This improves the error example output by reflecting the current state. It also makes sure the examples are up to date in general. `assert.throws` clarified the `ERR_AMBIGUOUS_ARGUMENT` error. PR-URL: #20313 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

docs: improve assert docs

c3ed6da

This improves the error example output by reflecting the current state. It also makes sure the examples are up to date in general. `assert.throws` clarified the `ERR_AMBIGUOUS_ARGUMENT` error.

BridgeAR requested review from Trott and vsemozhetbyt April 26, 2018 01:10

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Apr 26, 2018

BridgeAR commented Apr 26, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

fixup: clearer example

1391428

vsemozhetbyt approved these changes Apr 26, 2018

View reviewed changes

Trott approved these changes Apr 26, 2018

View reviewed changes

fixup: address comments

2da7521

trivikr approved these changes Apr 26, 2018

View reviewed changes

Trott reviewed Apr 26, 2018

View reviewed changes

fixup: address comments and move example

8fd1fe3

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2018

trivikr approved these changes Apr 28, 2018

View reviewed changes

BridgeAR closed this Apr 29, 2018

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

Uh oh!

Conversation

BridgeAR commented Apr 26, 2018

Checklist

Uh oh!

BridgeAR Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 26, 2018

Uh oh!

vsemozhetbyt commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Apr 26, 2018

Uh oh!

This comment has been minimized.

BridgeAR commented Apr 26, 2018

Uh oh!

vsemozhetbyt commented Apr 26, 2018

Uh oh!

BridgeAR commented Apr 26, 2018

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 26, 2018

Uh oh!

trivikr Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 27, 2018

Uh oh!

BridgeAR commented Apr 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vsemozhetbyt commented Apr 26, 2018 •

edited

Loading

vsemozhetbyt Apr 26, 2018 •

edited

Loading