dubzzz / fast-check

Property based testing framework for JavaScript (like QuickCheck) written in TypeScript
https://fast-check.dev/
MIT License
4.33k stars 181 forks source link

Add fc.assertProperty function #500

Closed ethanresnick closed 4 years ago

ethanresnick commented 4 years ago

🚀 Feature Request

Add a function fc.assertProperty(...args) that's equivalent to fc.assert(fc.property(...args)). For consistency, there should perhaps also be fc.checkProperty, equivalent to fc.check(fc.property(...args)).

Motivation

Pretty much all of my tests look like fc.assert(fc.property(...args)). It'd be nice to avoid the extra layer of nesting and just have an assertProperty function for these common cases. I migrated from js-verify to fast-check, and love it so far, but the lack of an analog to js-verify's assertforall has been a bit of a nuisance.

While I could easily define this function myself, it's actually a little messy to define its type, since fc.property has so many overloaded signatures, and TS offers no easy way to have my wrapper function inherit all those overloads, so I'd have to copy and paste them.

Example

it('should satisfy some property', () => {
  fc.assertProperty(fc.boolean(), (bool) => true);
}
dubzzz commented 4 years ago

Thanks a lot for the suggestion. This is indeed a recurring question or suggestion for the lib. And I really think it will be useful to come with it quickly.

IMO one cool thing would be to directly provide test wrappers, such as it or test, with property based capabilities.

My target will be to provide some kind of itProp that could be used as follow:

import { itProp, fc } from "jest-fast-check"; // not yet published
itProp('should satisfy some property', [fc.boolean()], () => true);

It would directly wrap it, fc.assert and fc.property behind a single function.

I initially drafted this suggestion for ava but I'll soon implement it for others (see https://github.com/dubzzz/ava-fast-check/).

@ethanresnick Would it fit your need? If so I'll come back to you as soon as I get an implementation for other test runners.

ethanresnick commented 4 years ago

Thanks @dubzzz, that would fit my need in most cases, and would be even more concise than what I proposed.

There might still be occassional edge cases where having just an assertProperty function would be useful, e.g.:

  1. A single test that requires setting up some state before the property check runs, e.g.:

    it('should x, y,z', () => {
       // setup state _just for this test_, then...
       fc.assertProperty(....)
    })

    In that case, using assertProperty would be more concise than having to wrap the single test in a describe block so that you can use a before hook to set up the state.

    The above scenario might be pretty rare, though, because usually I think you're either setting up common state for multiple tests at once (in which case those tests are already wrapped in a describe block with a before hook), or you want to set up the state in a deterministically random way, in which case you'd do it within the property's callback.

  2. If someone's using a test runner that you haven't written a wrapper for yet.

I don't know if those use cases are enough to merit having both itProp and assertProperty, but, either way, I look forward to having itProp a lot!

jdmarshall commented 4 years ago

So first of all, Mocha suggests you not use arrow functions if you want to be able to utilize mocha internals at all, which I expect you might.

I think the interface could be even simpler than suggested above. If I work from the project description toward an implementation instead of vice versa, then I think tests would look something like this:

let fc = require("fast-check");
let all = fc.all;

describe("something()", function() {
    all("should satisfy some constraint", function(a = fc.boolean(), b = fc.boolean()) {
        let reply = something(a, b);    
        expect(reply).to.be.true;
    });
});

That's it. All the magic is on one line. Everything else is bog-standard mocha, jest, or whatever. Nothing new to learn, no weird stack traces or unusual breakpoint stepping.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

dubzzz commented 4 years ago

Done for Jest, you may have a look to jest-fast-check. It provides an integration of fast-check with jest.

No more need to choose between property or asyncProperty, assert and property are merged together...

dubzzz commented 4 years ago

@jdmarshall The all suggestion looks great for mocha, but it has one main issue: it ties fast-check to test runners. If I'm not wrong it would require to call it in the case of mocha or jest, something else for others. Today fast-check can be executed anywhere, I prefer not to tied it to any external libs even test runners (interfacing with 'all test runners' at once is a high chance of bug reports because 1. difficult because it means also it.only and all variants, 2. they all have different approaches to test).

Moreover, every test runner has its own philosophy. The syntax you suggested might be great for mocha but it does not fit for ava (see ava-fast-check) or jest (where I will work on a custom expect, watch mode for quick replays...) for instance.

As far as I can see it, the assertProperty is more or less a shorthand for (...args) => assert(property(...args)) at first place. Some other args might be needed like: the settings for assert function, beforeEach and afterEach if any...

jdmarshall commented 4 years ago

It doesn’t tie it to the runner. It ties it to how default parameters are processed.

But I don’t see how you can be test runner agnostic anyway, since in the end you have to hijack the runner process or you only get one combination per test, which won’t be anywhere near enough to guarantee any kind of safety of the code.

False confidence from a test suite is really, really dangerous,

dubzzz commented 4 years ago

But I don’t see how you can be test runner agnostic

For the moment it is the case.

fast-check does not hijack the runner process by any means but it allows you to test many combinations per it without losing safety. It is not bind to any runner, and does not know anything about them.

When you write code like:

test('test name', () => { // test runner world
  fc.assert(/*... */) // fast-check world, running against multiple combinations 
}) 

It does not seem that fast-check needs to be knowledgeable about the runner it runs into?

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

nmay231 commented 1 year ago

Any possibility of reopening this?

I also think that "replacing" it() with testProp() is not the best API because some editor extensions might not detect you have a test on that line (I haven't tested it, but I feel like they only check for it() and test() calls, if I had to guess).

Notice the green checkmarks in the gutter on the left. They tell me whether the test(s) passed or failed and allow me to rerun the tests by clicking on them.

image

I think just having fc.assertProperty() that combines the two set of parameters would be best.

fc.assertProperty([...arbitraries], () => { ... }, { ...runTimeOptions });
nmay231 commented 1 year ago

FYI, @jdmarshall named arguments in javascript is a straight-up lie. They are still just ordered arguments

function test(a, b) {
  console.log(a, b)
}

test(a=1, b=2) // "1 2"
test(b=1, a=2) // still "1 2"!
test(stupid=1, design=2) // Yep "1 2"
dubzzz commented 1 year ago

For jest, I propose a package called @fast-check/jest exposing a it.prop and a test.prop. Same for Vitest.

Otherwise, no plan to implement nor maintain such assertProperty helper on the core part of fast-check.

nmay231 commented 1 year ago

I tried using it.prop from @fast-check/vitest, and it doesn't quite work for me.

For one thing, the test description is not the first argument ruining any reading out of the test description (it("has these properties", () => { ... })). Second, it doesn't play well with the eslint-plugin-vitest since the plugin doesn't understand it() from @fast-check/vitest is just a wrapper around the regular it(). That can be considered a shortcoming of the plugin, but I don't think so.

I can understand not wanting to support another API into the core library, but I hope this gives you more food for thought.

For now, I've written my own wrapper that mimics the form of a promise .then() to chain (.assertProperty()) for anyone interested. It's not perfect, but it helps with one level of indentation and moves the parameters object to the beginning of the test (let's learn from setTimeout's mistake of putting the time at the end).

// Usage
it("wraps fc.assert(fc.property())", () => {
    given([fc.integer(), fc.integer()]).assertProperty((a, b) => {
        expect(a).toBe(b);
    });

    given([fc.integer(), fc.integer()], {
        seed: -1109731515,
        path: "0:0:1:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0",
        endOnFailure: true,
    }).assertProperty((a, b) => {
        expect(a).toBe(b);
    });
});

Definition of given():

/**
 * A simple wrapper around fc.assert() to help remove indentation and move
 * params to the top of your test.
 *
 * ```ts
 * given(arbitraries, params).assertProperty(predicate)
 * ```
 *
 * is equivalent to
 *
 * ```ts
 * fc.assert(fc.property(...arbitraries, predicate), params)
 * ```
 */
export function given<Ts extends [unknown, ...unknown[]]>(
    arbitraries: { [K in keyof Ts]: fc.Arbitrary<Ts[K]> },
    params?: fc.Parameters<Ts>,
) {
    return {
        assertProperty: (predicate: (...args: Ts) => boolean | void) =>
            fc.assert(fc.property(...arbitraries, predicate), params),
    };
}
And some accompanying tests. ```typescript describe("given", () => { beforeAll(() => { vi.mock("fast-check", async () => { const fc = (await vi.importActual("fast-check")) as any; return { default: { ...fc, assert: vi.fn().mockReturnValue("assert"), property: vi.fn().mockReturnValue("property"), }, }; }); }); afterAll(() => { vi.clearAllMocks(); }); it("wraps fc.assert correctly when given no runtime parameters", () => { const predicate = vi.fn(); const arbitraries = [fc.integer(), fc.string()]; given(arbitraries as any).assertProperty(predicate); expect(fc.property as Mock).toBeCalledWith(...arbitraries, predicate); expect(fc.assert as Mock).toBeCalledWith("property", undefined); }); it("wraps fc.assert correctly when given runtime parameters", () => { const predicate = vi.fn(); const arbitraries = [fc.string(), fc.integer()]; const parameters = { numRuns: 42 }; given(arbitraries as any, parameters).assertProperty(predicate); expect(fc.property as Mock).toBeCalledWith(...arbitraries, predicate); expect(fc.assert as Mock).toBeCalledWith("property", { numRuns: 42 }); }); }); ```

EDIT: I moved the parameters to be after the arbitraries array in my personal codebase, and decided to update my example here.

jdmarshall commented 1 year ago

@dubzzz I believe I was referring to:

import { itProp, fc } from "jest-fast-check"; // not yet published itProp('should satisfy some property', [fc.boolean()], () => true);

If that's not integrating with the test runner then I don't know what is.

dubzzz commented 1 year ago

Definitely @fast-check/jest and it's legacy version fast-check-jest are binding test runners and fast-check together. I have not done that for mocha given the cost of adding, maintaining... such package.

ethanresnick commented 1 year ago

@dubzzz I know this is a closed issue, and of course it's ultimately your decision what to do, but I do think the current solution is not ideal.

In order for users to get the better DX offered by itProp and similar, you (or someone) has to maintain a fast-check plugin for every popular test runner, which doesn't seem practical (as evidenced by your comment about mocha). Then, users have to find/install a plugin for their runner and, even after all that, there are gonna be edge cases that cause trouble — like @nmay231's comments about how these plugins can interact badly with editors and eslint rules.

Having some shorthand methods built-in to fast-check core seems like a useful addition so that people using a test runner for which there is no fast-check plugin (or the plugin isn't updated, or it causes issues with some part of their workflow) can still benefit.

These shorthand methods could coexist with the existing plugins, but they would raise the baseline DX for people who don't use a plugin.

jdmarshall commented 11 months ago

There's absolutely no reason that the entry point into an addon for a testing library should have 2 nested calls to the same library in each line. Especially the same two calls. Chai doesn't do this. Sinon doesn't do this. No unit testing library addon I've ever used has worked this way.

Only FC.

dubzzz commented 11 months ago

There's absolutely no reason

There is! Otherwise I'd have addressed the problem when it first got reported and probably even before.

First we should not have one but two shorthands. One for synchronous predicates and another for asynchronous ones. You may consider: let's put everything asynchronous, I agree, but it's imply a huge performance penalty for synchronous code.

Second we not only have assert, but also check, sample or statistics. They all consume a property.

Third it mixes concerns.

Fourth, not only the three points above but we also have to define an API merging the two together, providing migration paths, no more ability to wrap properties externally (see @fast-check/worker project)...

If the need is that critical for you, I'd recommend you to implement your own assertProperty in your code (making the glue between the two) and to call it. For now, I have no plan to add it.

ewinslow commented 11 months ago

For me, I opted to create some custom glue for our test framework that does: it + fc.assert + fc.asyncProperty + fc.gen all in one. It's nice. fc.assertProperty would not be enough anyways, so good on @dubzzz for choosing to focus on composable primitives.