search

jqwik-team / jqwik

Property-Based Testing on the JUnit Platform
http://jqwik.net
Eclipse Public License 2.0
576 stars 64 forks source link

Prefer for loop in CombinedShrinkable#createValues for better stacktrace readability #437

Closed vlsi closed 1 year ago

vlsi commented 1 year ago

Testing Problem

java.util.stream frames are unreadable, and it makes it harder to tell why the test has stuck.

Sample stacktrace:

    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(java.base@11.0.13/ReduceOps.java:913)
    at java.util.stream.AbstractPipeline.evaluate(java.base@11.0.13/AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(java.base@11.0.13/ReferencePipeline.java:578)
    at net.jqwik.engine.properties.shrinking.ShrinkableContainer.createValue(ShrinkableContainer.java:30)
    at net.jqwik.engine.properties.shrinking.ShrinkableContainer.value(ShrinkableContainer.java:35)
    at net.jqwik.engine.properties.shrinking.ShrinkableString.value(ShrinkableString.java:10)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable$$Lambda$617/0x0000000800578840.apply(Unknown Source)
    at java.util.stream.ReferencePipeline$3$1.accept(java.base@11.0.13/ReferencePipeline.java:195)
    at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(java.base@11.0.13/ArrayList.java:1655)
    at java.util.stream.AbstractPipeline.copyInto(java.base@11.0.13/AbstractPipeline.java:484)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(java.base@11.0.13/AbstractPipeline.java:474)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(java.base@11.0.13/ReduceOps.java:913)
    at java.util.stream.AbstractPipeline.evaluate(java.base@11.0.13/AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(java.base@11.0.13/ReferencePipeline.java:578)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValues(CombinedShrinkable.java:29)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValue(CombinedShrinkable.java:25)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.value(CombinedShrinkable.java:21)
    at net.jqwik.engine.properties.shrinking.MappedShrinkable.value(MappedShrinkable.java:21)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable$$Lambda$617/0x0000000800578840.apply(Unknown Source)
    at java.util.stream.ReferencePipeline$3$1.accept(java.base@11.0.13/ReferencePipeline.java:195)
    at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(java.base@11.0.13/ArrayList.java:1655)
    at java.util.stream.AbstractPipeline.copyInto(java.base@11.0.13/AbstractPipeline.java:484)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(java.base@11.0.13/AbstractPipeline.java:474)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(java.base@11.0.13/ReduceOps.java:913)
    at java.util.stream.AbstractPipeline.evaluate(java.base@11.0.13/AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(java.base@11.0.13/ReferencePipeline.java:578)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValues(CombinedShrinkable.java:29)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValue(CombinedShrinkable.java:25)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.value(CombinedShrinkable.java:21)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable$$Lambda$617/0x0000000800578840.apply(Unknown Source)
    at java.util.stream.ReferencePipeline$3$1.accept(java.base@11.0.13/ReferencePipeline.java:195)
    at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(java.base@11.0.13/ArrayList.java:1655)
    at java.util.stream.AbstractPipeline.copyInto(java.base@11.0.13/AbstractPipeline.java:484)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(java.base@11.0.13/AbstractPipeline.java:474)
    at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(java.base@11.0.13/ReduceOps.java:913)
    at java.util.stream.AbstractPipeline.evaluate(java.base@11.0.13/AbstractPipeline.java:234)
    at java.util.stream.ReferencePipeline.collect(java.base@11.0.13/ReferencePipeline.java:578)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValues(CombinedShrinkable.java:29)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.createValue(CombinedShrinkable.java:25)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable.value(CombinedShrinkable.java:21)
    at net.jqwik.engine.properties.shrinking.FlatMappedShrinkable.value(FlatMappedShrinkable.java:80)
    at net.jqwik.engine.properties.shrinking.CombinedShrinkable$$Lambda$617/0x0000000800578840.apply(Unknown Source)

Suggested Solution

Rework CombinedShrinkable.createValues with for. The same for net.jqwik.engine.properties.shrinking.ShrinkableContainer#createValue.

jlink commented 1 year ago

The open question for me is: When is it worth using for loops instead of streams while blow up the code at the same time.

vlsi commented 1 year ago

Technically speaking, you could use the previous containerCollector(), so the code would be of the same length

vlsi commented 1 year ago

I think it should be fine to go for regular for loops in hot paths, especially, when the for loop is trivial. I would expect that arbitrary, generators, shinkers, and store would be on the hot paths.

For instance, in this change, you remove 4 lines, and replace them with 4-line-long for loop. IDEA does the transformation both ways, so if you ever want adding extra spice on top of the for loop, you can auto-replace it with stream in IDEA

jlink commented 1 year ago

My IDEA did not offer a for loop replacement for ShrinkableContainer.createValue(), and neither did Copilot suggest working code.

vlsi commented 1 year ago

Ah, looks like I was wrong, and custom collectors block autoreplacement.