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

Disable prettifying of large objects #2127

Open
charpov opened this issue May 10, 2022 · 8 comments
Open

Disable prettifying of large objects #2127

charpov opened this issue May 10, 2022 · 8 comments

Comments

@charpov
Copy link

charpov commented May 10, 2022

This is a feature request. It's a thing that's bitten me many times.

I have a test of the form:

assert(seq.length == 1_000_000)

When it fails, I'd be happy with:

Expected :1000000
Actual   :999999

Instead, the prettifier tries to build a humongous string and crashes the system with OME.

I'm getting by on a case-by-case with things like:

given Prettifier with
   def apply(o: Any): String = o.asMatchable match
      case col: Iterable[?] if col.size > 99 => Prettifier.default(col.take(100)).init + "..."
      case other                             => Prettifier.default(other)

but it'd be nice to have a more systematic solution. What's the point of generating strings so big no one will ever look at them anyway?

@cheeseng
Copy link
Contributor

cheeseng commented Jun 5, 2022

@charpov Sorry for replying late, I like your approach, not sure if we should make yet another argument for this, perhaps pass through config map is better, e.g. -Dprettifier.collection.max.size=... and we have a default value of 100. This may be breaking change for other tools that integrate ScalaTest though, so perhaps it is best to be done in 3.3 branch.

@charpov
Copy link
Author

charpov commented Jun 6, 2022

I'm glad to hear my code is not too unreasonable. It's only a partial solution, though. For instance, I have tests on large, randomly generated trees (implemented as case classes), that give me the same problem (and I use ScalaTest as a grading system in my teaching, so failed tests are quite common). Defining specific prettifiers is cumbersome, but I can't think of a more generic solution. Systematically truncating the result of toString would help avoid the multi-gigabyte logs I'm currently seeing, but it doesn't solve the problem of a single string that is too large for memory (or for the execution stack with recursive structures). Maybe toString in case classes shouldn't generate such large strings (or blow the stack), but there's nothing we can do about that.

@cheeseng
Copy link
Contributor

@charpov Fyi I included some improvement for this in the following PR that I just submitted:

#2131

I agree with you that we can't do much to control the toString, we may try to catch OutOfMemoryError (we are already catching StackOverflowError currently) and make it to return a fixed string like {LARGE_OBJECT}. In general it is not recommended to catch OOME except for resource cleaning purpose, but may be fine since we actually know the the spot that's causing it. What do you think?

@charpov
Copy link
Author

charpov commented Jun 13, 2022

I've tried for years to handle OOME in a systematic way (again, because I'm using ScalaTest as a grading system), but it's never worked. I've eventually given up. One issue is that there are many threads involved, and as one thread uses all the memory, other threads start to fail with OOME. Another issue I had is that my tests all have timeouts, and tests that fail with OOME tend to do it slowly and mess up timing anyway, even if I recover from the error.

BTW, I see that your code handles GenTraversable and GenMap. Haven't these traits been removed? Shouldn't you check for Iterable instead? If Iterable and Java collections are handled, that takes care of most cases, and only leaves the problem of large instances of cases classes, like my trees. For these, I will have to provide my own prettifier.

@cheeseng
Copy link
Contributor

@charpov You're right, I have submitted the following PR to do away with the deprecated GenTraversable and GenMap in Prettifier:

#2134

Yes OOME will occur in any active thread that is running, like you said it will be impossible to handle the failure in such scenario except cleaning up open resources. It could be of some use if the tests are running in single thread, which is not the usual case most of the time.

@charpov
Copy link
Author

charpov commented Oct 27, 2022

I see that you've added a truncating prettifier (thanks!). I'm still facing a difficulty when using custom prettifiers. For instance, in the failed test below, I use a prettifier to shorten the message ... did not equal ... (that part works), but then it's followed by an "Analysis" message that is not truncated. What is this "Analysis", and how do I get it to use my custom prettifier (or turn it off altogether)?

- large toPaths/fromPaths *** FAILED *** (176 milliseconds)
  Tree(78,Forest(ArraySeq(Tree(70,Forest(ArraySeq(Tree(50,Forest(ArraySeq())), Tree(62,Forest(ArraySeq(Tree(58,Forest(ArraySeq())), Tree(51,Forest(ArraySeq(Tree(69,Forest(ArraySeq())), Tree(98,Forest(ArraySeq())), Tree(81,Forest(ArraySeq(Tree(42,Forest(ArraySeq()))))), Tree(51,Forest(ArraySeq()))))), Tree(35,Forest(ArraySeq(Tree(91,Forest(ArraySeq(Tree(85,Forest(ArraySeq(Tree(20,Forest(ArraySeq(Tree(67,Forest(ArraySeq(Tree(24,Forest(ArraySeq(Tree(91,Forest(ArraySeq(Tree(83,Forest(ArraySeq())), Tree(54,Fore... did not equal Tree(78,Forest(Vector(Tree(70,Forest(Vector(Tree(50,Forest(Vector())), Tree(62,Forest(Vector(Tree(35,Forest(Vector(Tree(15,Forest(Vector(Tree(27,Forest(Vector(Tree(4,Forest(Vector(Tree(8,Forest(Vector())), Tree(5,Forest(Vector(Tree(55,Forest(Vector(Tree(91,Forest(Vector()))))), Tree(42,Forest(Vector(Tree(74,Forest(Vector(Tree(46,Forest(Vector()))))))))))), Tree(77,Forest(Vector(Tree(66,Forest(Vector(Tree(64,Forest(Vector(Tree(3,Forest(Vector())), Tree(91,Forest(Vector())), Tree(97,Forest(Vector())), Tree... (RandomTreeGradingSuite.scala:175)
  Analysis:
  Tree(forest: Forest(trees: ArraySeq$ofRef(0: Tree(70,Forest(ArraySeq(Tree(50,Forest(ArraySeq())), Tree(62,Forest(ArraySeq(Tree(58,Forest(ArraySeq())), Tree(51,Forest(ArraySeq(Tree(69,Forest(ArraySeq())), Tree(98,Forest(ArraySeq())), Tree(81,Forest(ArraySeq(Tree(42,Forest(ArraySeq()))))), Tree(51,Forest(ArraySeq()))))), Tree(35,Forest(ArraySeq(Tree(91,Forest(ArraySeq(Tree(85,Forest(ArraySeq(Tree(20,Forest(ArraySeq(Tree(67,Forest(ArraySeq(Tree(24,Forest(ArraySeq(Tree(91,Forest(ArraySeq(Tree(83,Forest(ArraySeq())), Tree(54,Forest(ArraySeq())), Tree(21,Forest(ArraySeq())), Tree(51,Forest(ArraySeq()))))), Tree(56,Forest(ArraySeq(Tree(20,Forest(ArraySeq())), Tree(75,Forest(ArraySeq())), Tree(6,Forest(ArraySeq()))))), Tree(9,Forest(ArraySeq(Tree(92,Forest(ArraySeq())), Tree(9,Forest(ArraySeq())))))))), Tree(75,Forest(ArraySeq())))))))))))))), Tree(74,Forest(ArraySeq())), Tree(15,Forest(ArraySeq(Tree(27,Forest(ArraySeq(Tree(92,Forest(ArraySeq(Tree(13,Forest(ArraySeq(Tree(48,Forest(ArraySeq(Tree(34,Forest(ArraySeq(Tree(61,Forest(ArraySeq())), Tree(57,Forest(ArraySeq())), Tree(2,Forest(ArraySeq())))))))), Tree(19,Forest(ArraySeq(Tree(89,Forest(ArraySeq(Tree(19,Forest(ArraySeq())), Tree(26,Forest(ArraySeq())), Tree(56,Forest(ArraySeq())))))))), Tree(39,Forest(ArraySeq(Tree(72,Forest(ArraySeq())), Tree(73,Forest(ArraySeq(Tree(20,Forest(ArraySeq())), Tree(31,Forest(ArraySeq())), Tree(27,Forest(ArraySeq())), Tree(62,Forest(ArraySeq())))))))))))))), Tree(4,Forest(ArraySeq(Tree(33,Forest(ArraySeq(Tree(35,Forest(ArraySeq(Tree(19,Forest(ArraySeq(Tree(58,Forest(ArraySeq())), Tree(34,Forest(ArraySeq())), Tree(12,Forest(ArraySeq())), Tree(0,Forest(ArraySeq())))))))), Tree(22,Forest(ArraySeq(Tree(62,Forest(ArraySeq())), Tree(64,Forest(ArraySeq(Tree(20,Forest(ArraySeq())), Tree(16,Forest(ArraySeq()))))), Tree(36,Forest(ArraySeq(Tree(73,Forest(ArraySeq())), Tree(98,Forest(ArraySeq())), Tree(62,Forest(ArraySeq()))))))))))), Tree(77,Forest(ArraySeq(Tree(89,Forest(ArraySeq(Tree(86,Forest(ArraySeq())), Tree(41,Forest(ArraySeq(Tree(66,Forest(ArraySeq())))))))), Tree(66,Forest(ArraySeq(Tree(64,Forest(ArraySeq(Tree(52,Forest(ArraySeq())), Tree(97,Forest(ArraySeq())), Tree(91,Forest(ArraySeq())), Tree(3,Forest(ArraySeq()))))))))))), Tree(5,Forest(ArraySeq(Tree(42,Forest(ArraySeq(Tree(74,Forest(ArraySeq(Tree(46,Forest(ArraySeq())))))))), Tree(55,Forest(ArraySeq(Tree(91,Forest(ArraySeq())))))))), Tree(8,Forest(ArraySeq())))))))))))))))))))) -> Tree(70,Forest(Vector(Tree(50,Forest(Vector())), Tree(62,Forest(Vector(Tree(35,Forest(Vector(Tree(15,Forest(Vector(Tree(27,Forest(Vector(Tree(4,Forest(Vector(Tree(8,Forest(Vector())), Tree(5,Forest(Vector(Tree(55,Forest(Vector(Tree(91,Forest(Vector()))))), Tree(42,Forest(Vector(Tree(74,Forest(Vector(Tree(46,Forest(Vector()))))))))))), Tree(77,Forest(Vector(Tree(66,Forest(Vector(Tree(64,Forest(Vector(Tree(3,Forest(Vector())), Tree(91,Forest(Vector())), Tree(97,Forest(Vector())), Tree(52,Forest(Vector())))))))), Tree(89,Forest(Vector(Tree(41,Forest(Vector(Tree(66,Forest(Vector()))))), Tree(86,Forest(Vector())))))))), Tree(33,Forest(Vector(Tree(22,Forest(Vector(Tree(36,Forest(Vector(Tree(62,Forest(Vector())), Tree(98,Forest(Vector())), Tree(73,Forest(Vector()))))), Tree(64,Forest(Vector(Tree(16,Forest(Vector())), Tree(20,Forest(Vector()))))), Tree(62,Forest(Vector()))))), Tree(35,Forest(Vector(Tree(19,Forest(Vector(Tree(0,Forest(Vector())), Tree(12,Forest(Vector())), 
...

(I'm truncating the rest of the message, which is huge.)

@cheeseng
Copy link
Contributor

@charpov Hmm, it seems like the Differ that is producing the 'Analysis' part (diffSet) seems like not take into account of size limit of the passed in Prettifier (the element does not call the passed in Prettifier also):

https://github.com/scalatest/scalatest/blob/3.2.x-new/jvm/scalactic/src/main/scala/org/scalactic/Differ.scala#L118

I'll try to see if I can enhance Differ to support that.

Cheers.

@cheeseng
Copy link
Contributor

@charpov Fyi I just submitted the following PR to enhance Differ to address your issue:

#2177

Cheers.

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

No branches or pull requests

2 participants