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

improve propertyHelper for failure messages #1480

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

leobalter
Copy link
Member

Example:
Before: descriptor value should be 42 Expected SameValue(«1», «0») to be true
After: descriptor value should be 42


Note that Expected SameValue(«1», «0») is for the list of failures while checking the descriptor, as each error adds up to the list. The error message is confusing if the expected value is similar, e.g. 1:

descriptor value should be 42 Expected SameValue(«1», «0») to be true

Example:
Before: descriptor value should be 42 Expected SameValue(«1», «0») to be true
After: descriptor value should be 42
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn’t seem like an improvement; showing the actual value alongside the expected value seems pretty critical.

@leobalter
Copy link
Member Author

@ljharb this is exactly the problem, the assertion was using internal actual and expected values, not anything from the user input.

This assertion was only telling if it found any failures while checking the descriptor.

Note that we already use the same assertion message - through failures.join('; ') - to inform of a different value or other failures.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, i get it now :-)

@leobalter leobalter merged commit 222b86c into tc39:master Mar 8, 2018
@leobalter leobalter deleted the improve-harness branch March 8, 2018 17:12
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

Successfully merging this pull request may close these issues.

2 participants