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

Expose LLExpect.affirm function for flexibility #109

Open
timerg opened this issue Apr 29, 2022 · 4 comments
Open

Expose LLExpect.affirm function for flexibility #109

timerg opened this issue Apr 29, 2022 · 4 comments

Comments

@timerg
Copy link

timerg commented Apr 29, 2022

Currently the whole LLExpect are hidden from interface file, so does the affirm function.

In some use cases, I need to run jest expect validation inside a scope (such as promise).
Under these cases, it is hard to return the assertion to testPromise, and is inappropriate to feed result to testAsync callback.

This link is one of the situation I met.

So would it be feasible that the affirm function is exposed to the user?

@glennsl
Copy link
Owner

glennsl commented May 1, 2022

Hey. It's not all that obvious to me why testAsync wouldn't work here. From what I can tell there's just one assertion at the end. Seems like exactly what testAsync is for.

@timerg
Copy link
Author

timerg commented May 3, 2022

In fact there is more than one assertion (expect). The library is doing property test, which means there are many auto-generated test. The callback function will run several times with different input s and will stop when one of them fails or all success.

What I want to achieve is to wrap up the expectation with affirm, and allow the failure exception be handled by others.

@glennsl
Copy link
Owner

glennsl commented May 3, 2022

Hmm, I see. Not to dismiss the idea entirely, but this goes a bit against the philosophy of this library. The restriction is there to prevent exactly this kind of code, which I find it very hard to understand what is trying to do, let alone what it's actually going to do.

For property-based testing specifically, the idea is to instead generate all the data before-hand, then run a completely ordinary test on each of them (with testAll). This separation of concerns makes it easier to understand the test, because there's nothing special about it, as well as the isolated generation code, and makes it easier to migrate from ordinary tests to property-based testing.

So I guess the question is, what is it about the fast-check approach that is better than the testAll approach? And could there be other ways to achieve the same benefits without opening this can of worms?

@LoganGrier
Copy link
Contributor

I strongly disagree with @glennsl's argument.

When writing tests, all else being equal, I want to maximize the number of defects my tests detect, and minimize the total amount of engineering time my team and I spend writing tests, maintaining tests, and root-causing test failures. Generating test cases and running them with testAll increases engineering time without detecting any additional defects.

Generating the data beforehand involves creating a dummy test with a fast-check assertion. This “test” takes the data generated by fast-check and write it to a file.

Since there is no general way to create a Rescript value from a serialization of that value, for every value that’s used as a parameter of a fast-check property, we need to write custom deserialization code. This increases the engineering time to write tests.

Every time we update the interface for creating the value, we also have to update the deserialization code. This increases the engineering time to maintain tests.

Every time a test fails, we don't have access to automatic shrinking. This increases the engineering time to root-cause test failures.

The purported benefit of this all of this work is improved readability. All else being equal, readability is good since it reduces the engineering time to maintain tests. However, in this case, the readability benefits are marginal at best.

Here is a test case written using the approach you've proposed:

testAll("Addition is commutative",
  readFileToString("someFile")->customDeserializationFunction,
  args => {
    let (a, b) = args
    expect(a->FooBar.add(b))->toEqual(b->FooBar.add(a))
  },
)

Here is the same test written using affirm:

test("Addition is commutative", () => {
  Property.SyncUnit.assertProperty2(CustomArbs.fooBar(), CustomArbs.fooBar(), (a, b) => {
    expect(a->FooBar.add(b))->toEqual(b->FooBar.add(a))->LLExpect.affirm
  })
})

Another Bad Workaround

Another proposed workaround is to return a boolean inside the fast-check assertion, and to call Jest.pass after the fast-check property is asserted:

test("Addition is commutative", () => {
  Property.Sync.assertProperty2(CustomArbs.fooBar(), CustomArbs.fooBar(), (a, b) => {
    (a->FooBar.add(b))->FooBar.equals(b->FooBar.add(a))
  })
  pass
})

The problem with this approach is that when the assertion fails, the test doesn't print the diagnostic information that Jest.expect would. This increases the time it takes to root-cause the failure.

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

3 participants