Modify Prettifier.truncateAt to work on case classes #2155

mdedetrich · 2022-08-04T10:57:37Z

In the PR #2131, Prettifier.truncateAt was added which allows the truncating of massive collections when they are pretty printed by ScalaTest. Unfortunately the current Prettifier.truncateAt doesn't work if collection/s are wrapped inside a case class (in my case the data I generate is located within case classes).

Due to Scala having Product along with methods for iterating over fields in a case class, the implementation is quite straight forward (just apply prettify on every element in the productIterator).

Note that we DO NOT truncate the number of fields within the case class, only the values behind those fields.

cheeseng · 2022-08-04T14:00:06Z

jvm/scalactic/src/main/scala/org/scalactic/Prettifier.scala

@@ -277,6 +277,8 @@ private[scalactic] class TruncatingPrettifier(sizeLimit: SizeLimit) extends Defa
        else
          theToString
      // SKIP-SCALATESTJS,NATIVE-END
+      case caseClazz: Product =>
+        s"${caseClazz.productPrefix}(" + caseClazz.productIterator.map(prettify(_, processed + caseClazz.productIterator)).mkString(", ") + ")"


First of all thanks for the PR! To be consistent, I think productIterator here should follow the size limit, and the processed part should be processed + caseClazz. Also, I think it will be good idea to add this to the DefaultPrettifier also. :)

First of all thanks for the PR! To be consistent, I think productIterator here should follow the size limit

Are you sure about this? There can be un-intended consequences because the ordering is based on the how the fields are defined in the case class so the dropping of specific fields can have surprising behaviour. Also the amount of fields in a case case class is typically are not that large (as opposed to the actual data in the case class). Finally this behaviour would be quite surprising for me, as a user I wouldn't expect truncation to occur on the fields themselves, just the data.

and the processed part should be processed + caseClazz

Nice catch

Also, I think it will be good idea to add this to the DefaultPrettifier also. :)

Sure will do, its trivial to do so.

First of all thanks for the PR! To be consistent, I think productIterator here should follow the size limit

Are you sure about this? There can be un-intended consequences because the ordering is based on the how the fields are defined in the case class. Also the amount of fields in a case case class is typically are not that large (as opposed to the actual data in the case class). Finally this behaviour would be quite surprising for me, as a user I wouldn't expect truncation to occur on the fields themselves, just the data.

and the processed part should be processed + caseClazz

Nice catch

Also, I think it will be good idea to add this to the DefaultPrettifier also. :)

Sure will do, its trivial to do so.

Hmm, that's true. It is just that it seems in-consistent with other guys in the room, but I think your justification makes good sense. :)

mdedetrich · 2022-08-04T14:14:54Z

I have just rebased the original PR fixing the processed + caseClazz issue

cheeseng · 2022-08-04T14:21:48Z

@mdedetrich Do you mind to add it to DefaultPrettifier also?

mdedetrich · 2022-08-04T14:22:39Z

Do you mind to add it to DefaultPrettifier also?

Will do, doing it now.

mdedetrich · 2022-08-04T14:31:45Z

@cheeseng

Do you mind to add it to DefaultPrettifier also?

Done, also added a test for it with a case class that has an Array (a datastructure that scalatest prettifier does custom formatting for).

mdedetrich · 2022-08-04T14:39:15Z

I just noticed that // SKIP-SCALATESTJS,NATIVE-END is in the wrong position after this PR, should I fix this?

cheeseng · 2022-08-04T14:41:38Z

I just noticed that // SKIP-SCALATESTJS,NATIVE-END is in the wrong position after this PR, should I fix this?

Sure, that will be great.

mdedetrich · 2022-08-04T14:44:33Z

Sure, that will be great.

Done, rebased and pushed

mdedetrich · 2022-08-04T14:48:17Z

Sorry, the new case I think it should work for scala-js and scala-native, so the previous location was actually correct?

Ah yes, indeed you are right. Let me revert

mdedetrich · 2022-08-04T14:49:58Z

Okay, reverted and pushed.

cheeseng

Looks good to me now, let's trigger CI to build again.

Thanks!

mdedetrich · 2022-08-04T19:53:06Z

@cheeseng The tests are failing but it seems to be unrelated?

cheeseng · 2022-08-05T01:25:03Z

@mdedetrich Actually it does seem to the related:

 "MyFile(["test", true, ]false) was not file" was not equal to "MyFile([test,true,]false) was not file" (BeWordSpec.scala:887)

As DefaultPrettifier will prettify the "test" string nicely now with your enhancement but our tests are still checking for the default case class toString. I'll see how I can help with that.

mdedetrich · 2022-08-05T06:17:57Z

@cheeseng

Ah yes indeed you are right. It seems to be pretty trivial to fix so I can also do it, let me know.

cheeseng · 2022-08-05T06:35:27Z

@mdedetrich Unfortunately I am currently working on another PR, I think it will be faster if you can help to fix failing tests, fyi you should be able to run the tests and see the failures by:

> sbt clean test

mdedetrich · 2022-08-05T06:36:55Z

@cheeseng

No worries, its an easy change and was an oversight on my part. I will update the PR with fixes to the tests by EOD.

mdedetrich · 2022-08-05T10:23:30Z

@cheeseng So I started going through and fixing some tests and noticed that for some cases there are unintended consequences, specifically if the case class implements its own .toString method. One good example of this is the Spread case class in TripleEqualsSupport. It defines its own .toString, i.e.

override def toString: String = Prettifier.default(pivot) + " +- " + Prettifier.default(tolerance)

and it was this .toString that used to be called before adding my change to the DefaultPrettifier. With my change the following happens

"not equal [Spread(3, 1)]" was not equal to "not equal [3 +- 1]"
ScalaTestFailureLocation: org.scalatest.matchers.dsl.NotWordSpec at (NotWordSpec.scala:306)
Expected :"not equal [3 +- 1]"
Actual   :"not equal [Spread(3, 1)]"

I guess the following options are

Specifically drop my change in DefaulPrettifier
If possible to figure out if there is a way to see if a case class overrides .toString and if so then use the .toString rather than my implementation
Move any custom .toString code in scalatest into DefaultPrettifer when we match against type.
Accept the consequences of this change???

wdyt?

cheeseng · 2022-08-05T13:41:44Z

@cheeseng So I started going through and fixing some tests and noticed that for some cases there are unintended consequences, specifically if the case class implements its own .toString method. One good example of this is the Spread case class in TripleEqualsSupport. It defines its own .toString, i.e.
override def toString: String = Prettifier.default(pivot) + " +- " + Prettifier.default(tolerance)
and it was this .toString that used to be called before adding my change to the DefaultPrettifier. With my change the following happens
"not equal [Spread(3, 1)]" was not equal to "not equal [3 +- 1]"
ScalaTestFailureLocation: org.scalatest.matchers.dsl.NotWordSpec at (NotWordSpec.scala:306)
Expected :"not equal [3 +- 1]"
Actual   :"not equal [Spread(3, 1)]"
I guess the following options are

Specifically drop my change in DefaulPrettifier

If possible to figure out if there is a way to see if a case class overrides .toString and if so then use the .toString rather than my implementation

Move any custom .toString code in scalatest into DefaultPrettifer when we match against type.

Accept the consequences of this change???

wdyt?

Hmm, that's a good point. I think a 'good enough' solution for option 2 is to check if the toString is starting with class name followed by a (, if it does we can use your implementation, if not we can just use its toString. What do you think?

mdedetrich · 2022-08-05T14:01:07Z

Hmm, that's a good point. I think a 'good enough' solution for option 2 is to check if the toString is starting with class name followed by a (, if it does we can use your implementation, if not we can just use its toString. What do you think?

To me this seems to much of a hack/workaround, https://stackoverflow.com/a/22867096 seems to be a more principled approach to this issue although tbh I am more of a fan of either option 1 or option 3.

Either way I don't feel that strongly, so if you want to push for 2 with either of the solutions (checking for class name prefix or using the getMethod().getDeclaringClass() trick

cheeseng · 2022-08-05T14:07:20Z

Hmm, that's a good point. I think a 'good enough' solution for option 2 is to check if the toString is starting with class name followed by a (, if it does we can use your implementation, if not we can just use its toString. What do you think?

To me this seems to much of a hack/workaround, https://stackoverflow.com/a/22867096 seems to be a more principled approach to this issue although tbh I am more of a fan of either option 1 or option 3.

Either way I don't feel that strongly, so if you want to push for 2 with either of the solutions (checking for class name prefix or using the getMethod().getDeclaringClass() trick

getMethod().getDeclaringClass() sounds like a good idea, but I am not sure if it will work with scala-js and scala-native, you may give that a shot, fyi to run scalactic tests you may do:

> sbt scalacticTestJS/test

mdedetrich · 2022-08-05T14:09:23Z

getMethod().getDeclaringClass() sounds like a good idea, but I am not sure if it will work with scala-js and scala-native, you may give that a shot, fyi to run scalactic tests you may do:

Thanks, I can also use the prefix case class as a fallback implementation for JS/native with the proper version (getMethod().getDeclaringClass()) for JVM. Do you have a currently existing example somewhere in scalatest where you provide different implementations for different platforms (JVM/JS/Native)?

cheeseng · 2022-08-05T14:22:44Z

@mdedetrich Actually I wonder if getMethod().getDeclaringClass() for case class will actually return the case class itself, afaik scala compiler generarte the toString for case class when compiling, so the toString declaring class can be the case class itself. Anyway it will be an interesting thing to try.

You may use // SKIP-SCALATESTJS,NATIVE-START and // SKIP-SCALATESTJS,NATIVE-END to skip a block of code for scala-js and scala-native, then use //SCALATESTJS,NATIVE-ONLY to include a line for scala-js and scala-native only code (you'll need to do it for everyline for multiline code).

mdedetrich · 2022-08-06T18:39:47Z

Actually I wonder if getMethod().getDeclaringClass() for case class will actually return the case class itself, afaik scala compiler generarte the toString for case class when compiling, so the toString declaring class can be the case class itself. Anyway it will be an interesting thing to try.

@cheeseng So indeed you are correct, since the Scala compiler automatically creates the .toString for a case class its already overridden by default, instead I have used the prefix method you suggested previously

I have just rebased and pushed the PR fixing up all of the tests, below is a summary of the changes

As discussed, detect if a case class has a custom .toString by checking the start of .toString with caseclazz.productPrefix with DefaultPrettifier.
- Comments have also been added to clearly explain why DefaultPrettifier detects for a custom .toString and TruncatingPrettifier does not.
The pattern match inside of DefaultPrettifier/TruncatedPrettifier to detect for case class was changed from case caseClazz: Product => to case caseClazz: Product if caseClazz.productArity != 0 =>. This is because case objects such as None are technically also a Product which caused them to be serialized into something different (i.e. None() instead of None).
- This also results in better performance since it shortcuts the main prettifier logic
Since before this PR a lot of the tests relied on using the .toString method (typically implicitly when a variable is used inside a string) I have created new variables named "Prettified" which contains the new prettified version and placed those variables using Scala's standard string interpolation.
- This was done due to the large amount of mechanical changes and to reduce the diff of the commit as much as possible
- An exception to this is StringReporterSuite since the test was already using triplequotes (""") so putting in another double quote was trivial
Some tests called it("should have pretty toString") was specifically asserting the .toString, this was renamed to it("should have pretty via DefaultPrettifier") to be more accurate

cheeseng · 2022-08-07T00:33:53Z

@mdedetrich Looks good to me! What a big set of changes! Started CI build again.

cheeseng · 2022-08-07T04:41:51Z

@mdedetrich Unfortunately Scala 2.10 seems doesn't like this:

[error] /home/cheeseng/git/scalatest-mdedetrich/jvm/scalatest-test/src/test/scala/org/scalatest/ShouldHavePropertiesSpec.scala:252:81: ')' expected but string literal found.
[error]         assert(caught1.getMessage === s"The author property had value \"Dickens\", instead of its expected value \"Gibson\", on object $bookPrettified")
[error]                                                                                 ^
[info] - should send an AlertProvided event for an alert in main spec body (1 millisecond)
[error] /home/cheeseng/git/scalatest-mdedetrich/jvm/scalatest-test/src/test/scala/org/scalatest/ShouldHavePropertiesSpec.scala:271:81: ')' expected but string literal found.
[error]         assert(caught1.getMessage === s"The author property had value \"Dickens\", instead of its expected value \"Gibson\", on object $bookPrettified")
[error]                                                                                 ^
[error] /home/cheeseng/git/scalatest-mdedetrich/jvm/scalatest-test/src/test/scala/org/scalatest/ShouldHavePropertiesSpec.scala:299:94: ')' expected but string literal found.
[error]         assert(caught1.getMessage === s"The author property had its expected value \"Dickens\", on object $bookPrettified")
[error]                                                                                              ^
[error] /home/cheeseng/git/scalatest-mdedetrich/jvm/scalatest-test/src/test/scala/org/scalatest/ShouldHavePropertiesSpec.scala:355:106: ')' expected but string literal found.
[error]         assert(caught1.getMessage === s"The title property had its expected value \"A Tale of Two Cities\", on object $bookPrettified, but the author property had value \"Dickens\", instead of its expected value \"Melville\", on object $bookPrettified")
[error]                                                                                                          ^
[error] /home/cheeseng/git/scalatest-mdedetrich/jvm/scalatest-test/src/test/scala/org/scalatest/ShouldHavePropertiesSpec.scala:427:94: ')' expected but string literal found.
[error]         assert(caught21.getMessage === s"The title property had value \"A Tale of Two Cities\", instead of its expected value \"Moby Dick\", on object $bookPrettified, and the author property had value \"Dickens\", instead of its expected value \"Melville\", on object $bookPrettified")
[error]                                                                                              ^
[error] /home/cheeseng/git/scalatest-mdedetrich/jvm/scalatest-test/src/test/scala/org/scalatest/ShouldHavePropertiesSpec.scala:453:106: ')' expected but string literal found.
[error]         assert(caught1.getMessage === s"The title property had its expected value \"A Tale of Two Cities\", on object $bookPrettified")
[error]                                                                                                          ^
[info] - should send an AlertProvided event for an alert in scope body (2 milliseconds)
[error] /home/cheeseng/git/scalatest-mdedetrich/jvm/scalatest-test/src/test/scala/org/scalatest/ShouldHavePropertiesSpec.scala:561:107: ')' expected but string literal found.
[error]         assert(caught21.getMessage === s"The title property had its expected value \"A Tale of Two Cities\", on object $bookPrettified, and the author property had its expected value \"Dickens\", on object $bookPrettified")
[error]

Quick fix perhaps just revert it back to string concat with +?

mdedetrich · 2022-08-07T07:17:49Z

@cheeseng Oh wow, didn't realize you still compiled for Scala 2.10. Yes I will remove the string interpolation and use concatenation.

cheeseng · 2022-08-07T07:19:22Z

@mdedetrich Yes we're only dropping scala 2.10 build in the upcoming 3.3 release. :)

mdedetrich · 2022-08-07T08:44:12Z

@cheeseng Just pushed a rebased PR removing the string interpolation in ShouldHavePropertiesSpec and I confirmed that locally the tests are passing with sbt ++2.10.7 test.

cheeseng · 2022-08-07T08:47:08Z

@mdedetrich I have just restarted the CI, good luck!

mdedetrich · 2022-08-07T09:31:08Z

@cheesung The tests have passed so all seems good now!

cheeseng · 2022-08-07T09:49:14Z

Thanks @mdedetrich !

@bvenners If this looks good to you please kindly approve and I'll merge it.

Thanks!

mdedetrich · 2022-08-11T14:32:06Z

@cheeseng Many thanks for the merge! Would it be possible to ping me on this thread when the ScalaTest RC/milestone is released with this change?

cheeseng · 2022-08-19T13:57:46Z

@mdedetrich Sorry I forgot to response here, yes I can try my best to ping you here when the next version is released, thanks again for your contributions!

cheeseng · 2022-09-30T01:29:02Z

@mdedetrich fyi 3.2.14 is released now.

Cheers.

mdedetrich · 2022-09-30T06:46:54Z

Awesome, thanks!

asmarcz · 2023-02-02T17:56:49Z

Actually I wonder if getMethod().getDeclaringClass() for case class will actually return the case class itself, afaik scala compiler generarte the toString for case class when compiling, so the toString declaring class can be the case class itself. Anyway it will be an interesting thing to try.

@cheeseng So indeed you are correct, since the Scala compiler automatically creates the .toString for a case class its already overridden by default, instead I have used the prefix method you suggested previously

I have just rebased and pushed the PR fixing up all of the tests, below is a summary of the changes

* As discussed, detect if a `case class` has a custom `.toString` by checking the start of `.toString` with `caseclazz.productPrefix` with `DefaultPrettifier`.
  
  * Comments have also been added to clearly explain why `DefaultPrettifier` detects for a custom `.toString` and `TruncatingPrettifier` does not.

* The pattern match inside of `DefaultPrettifier`/`TruncatedPrettifier` to detect for case class was changed from `case caseClazz: Product =>` to `case caseClazz: Product if caseClazz.productArity != 0 =>`. This is because `case objects` such as `None` are technically also a `Product` which caused them to be serialized into something different (i.e. `None()` instead of `None`).
  
  * This also results in better performance since it shortcuts the main prettifier logic

* Since before this PR a lot of the tests relied on using the `.toString` method (typically implicitly when a variable is used inside a string) I have created new variables named "Prettified" which contains the new prettified version and placed those variables using Scala's standard string interpolation.
  
  * This was done due to the large amount of mechanical changes and to reduce the diff of the commit as much as possible
  * An exception to this is `StringReporterSuite` since the test was already using triplequotes (`"""`) so putting in another double quote was trivial

* Some tests called `it("should have pretty toString")` was specifically asserting the `.toString`, this was renamed to `it("should have pretty via DefaultPrettifier")` to be more accurate

Hello, I have just encountered the toString prefix checking and it has made me doubt my sanity for a moment. Is there any way to circumvent it? My use case is very simple:

case class LD(coordinates: (Int, Int))
// LD((0, 0)) -> LD(0, 0)

Thank you

cla-bot bot added the cla-signed label Aug 4, 2022

mdedetrich mentioned this pull request Aug 4, 2022

Truncate excessively large log output in large collections Aiven-Open/guardian-for-apache-kafka#265

Merged

cheeseng requested changes Aug 4, 2022

View reviewed changes

mdedetrich force-pushed the also-truncate-case-class branch from 50ff18a to 417bf76 Compare August 4, 2022 14:14

mdedetrich force-pushed the also-truncate-case-class branch from c57a2d2 to 67665f7 Compare August 4, 2022 14:43

Modify Prettifier.truncateAt to work on case classes

8db0199

mdedetrich force-pushed the also-truncate-case-class branch from 67665f7 to 4f97740 Compare August 4, 2022 14:49

cheeseng approved these changes Aug 4, 2022

View reviewed changes

mdedetrich force-pushed the also-truncate-case-class branch from 4f97740 to 7fc9ae6 Compare August 6, 2022 18:39

mdedetrich force-pushed the also-truncate-case-class branch 2 times, most recently from 519887e to 6d84588 Compare August 6, 2022 18:51

Also add case class prettifying to DefaultPrettifier

5aebfe8

mdedetrich force-pushed the also-truncate-case-class branch from 6d84588 to 5aebfe8 Compare August 7, 2022 08:43

cheeseng requested a review from bvenners August 10, 2022 15:16

bvenners approved these changes Aug 11, 2022

View reviewed changes

cheeseng merged commit 5aebfe8 into scalatest:3.2.x-new Aug 11, 2022

mdedetrich deleted the also-truncate-case-class branch August 11, 2022 14:31

This was referenced Oct 22, 2022

Fix prettifier not being used for property check #2173

Merged

Prettifier not being used for property + future check #2174

Open

Modify Prettifier.truncateAt to work on case classes #2155

Modify Prettifier.truncateAt to work on case classes #2155

Conversation

mdedetrich commented Aug 4, 2022 • edited

cheeseng Aug 4, 2022

Choose a reason for hiding this comment

mdedetrich Aug 4, 2022 • edited

Choose a reason for hiding this comment

cheeseng Aug 4, 2022

Choose a reason for hiding this comment

mdedetrich commented Aug 4, 2022

cheeseng commented Aug 4, 2022

mdedetrich commented Aug 4, 2022

mdedetrich commented Aug 4, 2022 • edited

mdedetrich commented Aug 4, 2022

cheeseng commented Aug 4, 2022

mdedetrich commented Aug 4, 2022

mdedetrich commented Aug 4, 2022

mdedetrich commented Aug 4, 2022

cheeseng left a comment

Choose a reason for hiding this comment

mdedetrich commented Aug 4, 2022

cheeseng commented Aug 5, 2022

mdedetrich commented Aug 5, 2022

cheeseng commented Aug 5, 2022

mdedetrich commented Aug 5, 2022

mdedetrich commented Aug 5, 2022 • edited

cheeseng commented Aug 5, 2022

mdedetrich commented Aug 5, 2022

cheeseng commented Aug 5, 2022

mdedetrich commented Aug 5, 2022 • edited

cheeseng commented Aug 5, 2022

mdedetrich commented Aug 6, 2022 • edited

cheeseng commented Aug 7, 2022

cheeseng commented Aug 7, 2022

mdedetrich commented Aug 7, 2022

cheeseng commented Aug 7, 2022

mdedetrich commented Aug 7, 2022

cheeseng commented Aug 7, 2022

mdedetrich commented Aug 7, 2022

cheeseng commented Aug 7, 2022

mdedetrich commented Aug 11, 2022

cheeseng commented Aug 19, 2022

cheeseng commented Sep 30, 2022

mdedetrich commented Sep 30, 2022

asmarcz commented Feb 2, 2023

mdedetrich commented Aug 4, 2022 •

edited

mdedetrich Aug 4, 2022 •

edited

mdedetrich commented Aug 4, 2022 •

edited

mdedetrich commented Aug 5, 2022 •

edited

mdedetrich commented Aug 5, 2022 •

edited

mdedetrich commented Aug 6, 2022 •

edited