Add de.thetaphi.forbiddenapis to avoid usage of unwanted APIs

[vlsi]

See https://github.com/policeman-tools/forbidden-apis/wiki/GradleUsage
See https://github.com/policeman-tools/forbidden-apis/wiki/SignaturesSyntax

Overview

forbidden-apis allows identify the usage of unwanted APIs.
For instance, Collections#toSet can yield non-reproducible outcomes, and I believe it is important that Jqwik could reproduce the same generation and shrinking sequences based on the input seed only.

See one of the issues in #340

Details

There are out-of-the-box signatures that might be helpful: https://github.com/policeman-tools/forbidden-apis/wiki/BundledSignatures

---

I hereby agree to the terms of the jqwik Contributor Agreement.

[vlsi]

I did a few replacements (basically "replace structurally" in IDEA), and it seems like the change uncovers true bugs.

For instance:

JqwikExecutorTests > previouslyFailedTestsAreRunFirst FAILED
    org.mockito.exceptions.verification.VerificationInOrderFailure: 
    Verification in order failure
    Wanted but not invoked:
    engineExecutionListener.executionStarted(
        <Is property descriptor for>
    );
    -> at net.jqwik.engine.execution.JqwikExecutorTests.previouslyFailedTestsAreRunFirst(JqwikExecutorTests.java:34)
    Wanted anywhere AFTER following interaction:
    engineExecutionListener.executionStarted(
        PropertyMethodDescriptor: [engine:jqwik]/[class:net.jqwik.engine.execution.JqwikExecutorTests$TestContainer]/[property:test2()]
    );
    -> at net.jqwik.engine.execution.RecordingExecutionListener.executionStarted(RecordingExecutionListener.java:29)
        at app//net.jqwik.engine.execution.JqwikExecutorTests.previouslyFailedTestsAreRunFirst(JqwikExecutorTests.java:34)

I believe https://github.com/jlink/jqwik/blob/cc952102e3209a20c63e9b1cb6a7c3007a028b91/engine/src/main/java/net/jqwik/engine/execution/JqwikExecutor.java#L49-L51 should not iteratively try moving every test to the beginning of the queue, but it should keep the order of the input set.

It think it would be good to keep the order of the tests across executions.

[vlsi]

@jlink , now the PR passes the test for :engine.
However, it not clear what to do with the rest modules.

For instance, documentation and others might need toLinkedHashSet() collector, and it looks like net.jqwik.engine.support.JqwikCollectors won't be visible in other modules.
Duplicating Collectors.toLinkedHashSet() does not seem to be nice either.
I incline to add the collector to :api, however I would like to hear your opinion on all this.

> Task :documentation:forbiddenApisTest FAILED
Watching 402 directories to track changes
Caching disabled for task ':documentation:forbiddenApisTest' because:
  in net.jqwik.docs.contracts.eurocalc.RateProviderContractProperties$CurrencyConstraint$1 (RateProviderContractProperties.java:44)
Forbidden method invocation: java.util.stream.Collectors#toSet() [Prefer Collections.toCollection(LinkedHashSet::new) for reproducible order of the results]
  Build cache is disabled
  in net.jqwik.docs.state.mystore.MyStoreExamples$StoreChangesDetector (MyStoreExamples.java:107)
Forbidden method invocation: java.util.stream.Collectors#toSet() [Prefer Collections.toCollection(LinkedHashSet::new) for reproducible order of the results]
  in net.jqwik.docs.state.mystore.MyStore (MyStore.java:38)
Forbidden method invocation: java.util.stream.Collectors#toSet() [Prefer Collections.toCollection(LinkedHashSet::new) for reproducible order of the results]
  in net.jqwik.docs.state.mystore.MyStore (MyStore.java:45)
Scanned 178 class file(s) for forbidden API invocations (in 0.06s), 4 error(s).

[vlsi]

I realized I could add set element order stability tests into net.jqwik.api.edgeCases.GenericEdgeCasesProperties like in
b61ead3, then it jqwik will automatically test that across various Arbitrary implementations.

It even uncovers an issue in ArrayArbitraryTests, ListArbitraryTests, and MapArbitraryTests.
They somehow fail to pass mapEachProducesConsistentItemOrder even with all the sets and maps replaced with Linked alternatives.

WDYT regarding the new property right in GenericEdgeCasesProperties?

[vlsi]

If I adjust ListArbitraryTests to limit the generated list size to 3, I get the following:

Arbitrary<List<Integer>> arbitrary = ints.list().ofMaxSize(3); // <-- added ofMaxSize(3)

  org.opentest4j.AssertionFailedError:
    expected: "[-10, -10, -10]"
     but was: "[10, -10, 10]"

                              |-----------------------jqwik-----------------------
tries = 1                     | # of calls to property
checks = 1                    | # of not rejected calls
generation = RANDOMIZED       | parameters are randomly generated
after-failure = SAMPLE_FIRST  | try previously failed sample, then previous seed
when-fixed-seed = ALLOW       | fixing the random seed is allowed
edge-cases#mode = MIXIN       | edge cases are mixed in
edge-cases#total = 0          | # of all combined edge cases
edge-cases#tried = 0          | # of edge cases tried in current run
seed = -1982104500786880693   | random seed to reproduce generated values

Shrunk Sample (1 steps)
-----------------------
  arbitrary: net.jqwik.engine.properties.arbitraries.DefaultListArbitrary@2b9b7f1f
  transformation: identity
  random: java.util.Random@847f3e7
  size: 3
  genSize: 1
  withEdgeCases: true

Original Sample
---------------
  arbitrary: net.jqwik.engine.properties.arbitraries.DefaultListArbitrary@2b9b7f1f
  transformation: .set().flatMapEach(just(value))
  random: java.util.Random@847f3e7
  size: 3
  genSize: 2
  withEdgeCases: true

  Original Error
  --------------
  org.opentest4j.AssertionFailedError:
    expected: "[[], [-10, -10], [10, -10, 10], [-10], [10], [10, 10, -10], [-10, 10], [10, -10], [-10, 10, 10], [-10, -10, -10], [-10, -10, 10], [10, 10], [10, 10, 10], [-10, 10, -10], [10, -10, -10]]"
     but was: "[[], [-10, 10], [10], [-10, -10], [10, 10], [-10, 10, 10], [-10, -10, 10], [-10], [10, -10, -10], [10, 10, 10], [10, -10], [-10, 10, -10], [-10, -10, -10], [10, 10, -10], [10, -10, 10]]"

[vlsi]

@jlink , I think I found the reason ListArbitraryTests and other container-like arbitraries fail the reproducibility test.

net.jqwik.engine.properties.arbitraries.randomized.ContainerGenerator#noDuplicatesHadToBeSwitchedOff is a state value, which might be flipped at random during generation time, and it would affect subsequent generators.

In other words, the generated value depends on both: input random and past history (noDuplicatesHadToBeSwitchedOff activation because of TooManyFilterMissesException).

If I change private boolean noDuplicatesHadToBeSwitchedOff = true, the tests become much better.

[vlsi]

Wow. It looks like all the tests pass now.
I do not know why CI takes 10-15 min though.

I would suggest enabling Gradle Build Scan for the CI runs, and add #349

[jlink]

@vlsi Just noticed you've been adding a few commits lately, so a short heads-up: I've pushed off reviewing to make up my mind about two things:

A: Should LinkedHashSet really be used in all places where HashSet is currently used?

LHS uses more memory, so its use must be warranted.

I agree that LHS should replace HS whenever it's used as part of value generation and shrinking, and when it's beneficial to keep the order (eg the snippet you've given from JqwikExecutor). In other cases, I'd prefer to stick with plain HS; I haven't checked how much remains, though.
@vlsi Do you see additional reasons for using LHS instead of HS?

Since I already started to have some support stuff to the API module, having JqwikCollectors in there as well seems acceptable. Maybe a common naming scheme like net.jqwik.api.support.CollectorsSupport and marking it @API(status = INTERNAL) would be reasonable.

B: Should forbiddenapis be used to enforce the usage of the right API?

Given that the build is already rather involved and the unknown performance implication of the forbiddenapis plugin, I'd rather not add it to the build.

Adding tests (like you did in GenericEdgeCasesProperties) is a good thing. As discussed elsewhere, the repeatable consistent order is only guaranteed for freshly created generators.

BTW, #340 should be covered by this PR, right?

[vlsi]

Just noticed you've been adding a few commits lately

I just rebased since there were merge conflicts

[vlsi]

I'd prefer to stick with plain HS

How are you going to tell if HS is allowable in a given place or not?

I believe there are much more important questions, and there's no point in discussing HS vs LHS. The difference would most likely be negligible.

Given that the build is already rather involved and the unknown performance implication of the forbiddenapis plugin, I'd rather not add it to the build

Forbidden APIs build configuration takes less than 20 lines, and the verification takes less than 1 second (see https://scans.gradle.com/s/ob2emuhhhmvzi/timeline?sort=longest&type=de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis), so both items you highlight are not an issue really.

[vlsi]

#340 should be covered by this PR, right?

This PR fixes #340.

[vlsi]

As discussed elsewhere, the repeatable consistent order is only guaranteed for freshly created generators.

So far all the generators pass the stricter verification, so I would suggest keeping both verifications: compare repeatable generation with the same starting seed and generation with freshly created generators.

[jlink]

I believe there are much more important questions, and there's no point in discussing HS vs LHS. The difference would most likely be negligible.

I see a point. LHS is just the less common set implementation, it requires a motive to be used.

Given that the build is already rather involved and the unknown performance implication of the forbiddenapis plugin, I'd rather not add it to the build

Forbidden APIs build configuration takes less than 20 lines, and the verification takes less than 1 second (see https://scans.gradle.com/s/ob2emuhhhmvzi/timeline?sort=longest&type=de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis), so both items you highlight are not an issue really.

At one point or other I’ll need an exception from the rule and then have to fight the tool. IME those strict tools bring a lot of unwanted effort and gravity that I don’t like.

[vlsi]

LHS is just the less common set implementation, it requires a motive to be used

LHS is the default in Kotlin when you do mutableSetOf and friends, so it is not uncommon.

[jlink]

LHS is just the less common set implementation, it requires a motive to be used

LHS is the default in Kotlin when you do mutableSetOf and friends, so it is not uncommon.

Everyone’s entitled to their own default. In Java mine is HS.