test: refactor test-fs-buffer #11232

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@Trott
Member

Trott commented Feb 8, 2017

  • Remove unneeded temp dir cleanup
  • Add check for error in .close() callback
  • Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen

Now, they look like this:

AssertionError: 2a is hex value for * and not catch-stdout-error.js
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

@Trott Trott added fs test labels Feb 8, 2017

@jasnell

jasnell approved these changes Feb 8, 2017

test/parallel/test-fs-buffer.js
+ assert.strictEqual(
+ fromHexList,
+ val,
+ `${hexList[idx]} is hex value for ${fromHexList} and not ${val}`

This comment has been minimized.

@joyeecheung

joyeecheung Feb 8, 2017

Member

I think the more common wording would be something like

const valHex = Buffer.from(val).toString('hex');
const message = `expected ${valHex} as hex value for ${val}, ` +
                `got ${hexList[idx]} (hex value for ${fromHexList})`

If I am reading this correctly?

@joyeecheung

joyeecheung Feb 8, 2017

Member

I think the more common wording would be something like

const valHex = Buffer.from(val).toString('hex');
const message = `expected ${valHex} as hex value for ${val}, ` +
                `got ${hexList[idx]} (hex value for ${fromHexList})`

If I am reading this correctly?

This comment has been minimized.

@Trott

Trott Feb 8, 2017

Member

Your wording is better. Will update...

@Trott

Trott Feb 8, 2017

Member

Your wording is better. Will update...

This comment has been minimized.

@Trott

Trott Feb 8, 2017

Member

How about this?:

`expected ${val}, got ${fromHexList} by hex decoding ${hexList[idx]}`

The thing I'm trying to convey is this: val is the input. We put it in a file hex encoded. We read the file back in and got the hex-encoded value hexList[idx]. When we hex decode that value, we got fromHexList instead of val.

@Trott

Trott Feb 8, 2017

Member

How about this?:

`expected ${val}, got ${fromHexList} by hex decoding ${hexList[idx]}`

The thing I'm trying to convey is this: val is the input. We put it in a file hex encoded. We read the file back in and got the hex-encoded value hexList[idx]. When we hex decode that value, we got fromHexList instead of val.

This comment has been minimized.

@joyeecheung

joyeecheung Feb 9, 2017

Member

Yeah your proposal works too, and it is more succinct :)

@joyeecheung

joyeecheung Feb 9, 2017

Member

Yeah your proposal works too, and it is more succinct :)

Trott added some commits Feb 8, 2017

test: refactor test-fs-buffer
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: 2a is hex value for * and not catch-stdout-error.js
```
@Trott

This comment has been minimized.

Show comment
Hide comment

Trott added a commit to Trott/io.js that referenced this pull request Feb 10, 2017

test: refactor test-fs-buffer
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: nodejs#11232
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 10, 2017

Member

Landed in 05be623

Member

Trott commented Feb 10, 2017

Landed in 05be623

@Trott Trott closed this Feb 10, 2017

italoacasas added a commit that referenced this pull request Feb 13, 2017

test: refactor test-fs-buffer
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: #11232
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: refactor test-fs-buffer
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: nodejs#11232
Reviewed-By: James M Snell <jasnell@gmail.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: refactor test-fs-buffer
* Remove unneeded temp dir cleanup
* Add check for error in `.close()` callback
* Improve error reporting

On that last bullet point, the previous version of the test reported
errors like this:

```
AssertionError: [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen deepStrictEqual [ '.empty-repl-history-file',
  '.node_repl_history',
  'GH-1899-output.js',
  'GH-892-request.js',
  'a.js',
  'a1.js',
  'agen
```

Now, they look like this:

```
AssertionError: expected *, got ! by hex decoding 2a
```

PR-URL: nodejs#11232
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

would need backport PRs to land in v4 or v6

Member

jasnell commented Mar 7, 2017

would need backport PRs to land in v4 or v6

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