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

Added custom printing of strings while Asserting different types #205

Closed
wants to merge 5 commits into from

Conversation

AarthiT
Copy link

@AarthiT AarthiT commented Oct 19, 2021

Description

The AssertionError given when asserting an Integer and a String of the same Integer is not intuitive enough as the error message contains the same value for both expected and actual args.

Example: listOf(1, 2, "fizz", 4, "buzz") shouldEqual listOf("1", "2", "fizz", 4, "buzz") gives the AssertionError as -
java.lang.AssertionError: Expected <[1, 2, fizz, 4, buzz]>, actual <[1, 2, fizz, 4, buzz]>.

The change made to solve this issue was update ComparisonFailedException to take expected and actual args with quotes if they are strings.

This closes the Issue #121 .

Checklist

  • I've added my name to the AUTHORS file, if it wasn't already present.

Copy link
Contributor

@javatarz javatarz left a comment

Choose a reason for hiding this comment

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

Hey @AarthiT, Thank you for your first contribution!

Could you work on a couple of minor updates I've requested?

@@ -215,5 +215,6 @@ class ComparisonFailedException(val customMessage: String?, val expected: String
""".trimMargin().trim()
) {
constructor(customMessage: String?, expected: Any?, actual: Any?)
: this(customMessage, expected?.toString(), actual?.toString())
: this(customMessage, if (expected is String) "\"" + expected.toString() + "\"" else expected?.toString(),
if (actual is String) "\"" + actual.toString() + "\"" else actual?.toString())
Copy link
Contributor

Choose a reason for hiding this comment

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

The expected and actual Strings are being modded the same way, can we pull a function here?

fun quoteIfString(value: Any?) = if (value is String) "\"$value\"" else value?.toString()

Might want to experiment with triple quotes to avoid the need for backslashes to see if that works better.

try {
"5" shouldBeEqualTo 5
} catch (e: ComparisonFailedException) {
e.message shouldBeEqualTo "Expected: <5> but was: <\"5\">"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the extra space after shouldBeEqualTo?

AUTHORS.md Outdated
1. Piotr Bakalarski [@piotrb5e3](https://github.com/piotrb5e3) ([Contributions](https://github.com/MarkusAmshove/Kluent/commits?author=piotrb5e3))
1. Aarthi T [@AarthiT](https://github.com/AarthiT) ([Contributions](https://github.com/MarkusAmshove/Kluent/commits?author=AarthiT))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an empty line after your entry? That’ll avoid the diff issue for others because it will always show only people's changes 😄

@AarthiT AarthiT changed the title Added custom printing of strings to easily distinguish single digit St… Added custom printing of strings while Asserting different types Oct 19, 2021
@javatarz
Copy link
Contributor

Hey @MarkusAmshove, this PR looks good to go from my end. Can you help trigger the build and approve the changes?

I can't do either of these right now 😞

@javatarz
Copy link
Contributor

@AarthiT: there's 4 failing tests on your PR. Can you have a look when you have a few moments and fix them as necessary?

@javatarz javatarz mentioned this pull request Oct 27, 2021
1 task
@MarkusAmshove
Copy link
Owner

Has been implemented with #207

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.

None yet

3 participants