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

test: check custom inspection truncation in assert #28234

Closed
wants to merge 1 commit into from

Conversation

@Trott
Copy link
Member

commented Jun 14, 2019

The assert module has some truncation logic in a custom inspect
function. This was not covered in tests. Add tests to cover it.

The previously-uncovered lines are

if (lines.length > 10) {
lines.length = 10;
this[name] = `${lines.join('\n')}\n...`;
} else if (this[name].length > 512) {
this[name] = `${this[name].slice(512)}...`;
}
.

@BridgeAR I could use some confirmation that this is covering a feature and not revealing a bug. When an ASSERTION_ERROR with a long line is inspected, the value in actual or expected will be truncated but the same value will appear in full in the message. Whoops? Or by design?

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@nodejs-github-bot

This comment was marked as outdated.

@Trott Trott added the assert label Jun 14, 2019

@Trott

This comment was marked as outdated.

Copy link
Member Author

commented Jun 14, 2019

@Trott

This comment was marked as outdated.

Copy link
Member Author

commented Jun 15, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@Trott

@BridgeAR I could use some confirmation that this is covering a feature and not revealing a bug. When an ASSERTION_ERROR with a long line is inspected, the value in actual or expected will be truncated but the same value will appear in full in the message. Whoops? Or by design?

It is by design: we used to only visualize the stack property in case of a fatal exception and other cases. I changed that recently to fully inspect errors on fatal exceptions. Now all properties on an error will be fully inspected. Since the actual and expected values will be visible in the message anyway, it's better to minimize the output for the properties by default, since it would otherwise duplicate the information and that seems pretty verbose. That was the original reason for me to limit the output for these properties.

@BridgeAR
Copy link
Member

left a comment

LGTM. Thanks for writing the tests for this!

@danbev
danbev approved these changes Jun 18, 2019
});

// Output that extends beyond 10 lines sould also be truncated for display.

This comment has been minimized.

Copy link
@danbev

danbev Jun 18, 2019

Member

Nit: sould -> should

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

test: check custom inspection truncation in assert
The assert module has some truncation logic in a custom inspect
function. This was not covered in tests. Add tests to cover it.

PR-URL: #28234
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

@Trott Trott force-pushed the Trott:check-truncation branch from 09888e3 to e3d5257 Jun 18, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

Landed in e3d5257

@Trott Trott closed this Jun 18, 2019

@Trott Trott deleted the Trott:check-truncation branch Jun 18, 2019

targos added a commit that referenced this pull request Jul 2, 2019
test: check custom inspection truncation in assert
The assert module has some truncation logic in a custom inspect
function. This was not covered in tests. Add tests to cover it.

PR-URL: #28234
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@targos targos referenced this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.