dubzzz / fast-check

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

How do I test concurrency for an API? #2780

Closed younes-io closed 2 years ago

younes-io commented 2 years ago

💬 Question and Help

I am developing a booking app and I want to use the fast-check library to simulate race conditions in my API. Basically, I want to simulate the use case where many users would call the same API to book the same table at the same time :

Suppose this is my API : POST /book with a request body like :


{
   "table": 1,
   "time": "20:00"
}

My koa API looks something like this:


router
    .post('/book', (ctx) => {
        const { table, time } = ctx.request.body;
        const { isBooked } = await checkIfAvailable(table,time).body; // calls MongoDB
        if(!isBooked){
            ctx.body = {
                table,
                time,
                booked: true
            };  
        } else {
            ctx.throw("This table is already booked", 404)
        }

    })

My test looks like this :


mockedCheckIfAvailable.mockImplementation(
    s.scheduleFunction(async (table,time) => {
        return  booking;
    }),
);

const  res  =  await  request
    .post('/book')
    .set('Content-Type', 'application/json')
    .set('Accept', 'application/json')
    .send(booking);
expect(res).toBe(true);

I read the Race Conditions docs but I can't get around how to apply it.

dubzzz commented 2 years ago

Hey,

You probably want to have a look into those examples on race conditions: https://github.com/dubzzz/fast-check/tree/main/example/005-race

They should be useful to see how it can be applied onto "real" examples. Just in case it can help, I also gave a talk at React Europe to introduce the technique: https://www.youtube.com/watch?v=GIPbY75-lEo.

Please let me know, if it is is still not clear with that resources. They might not be perfect!

younes-io commented 2 years ago

Hello @dubzzz ,

Thank you for the link. I have already watched that video, and I also read most of the docs of the library. The weird part is when I put the API call inside the fc.asyncProperty(), the test passes even though my assertion is expect(s.report()).toBe(5);... it always passes When I put the API call out of the fc.asyncProperty() block, the API call is made, and the test fails (which is what I expect).

So, my question is: does fast-check have an impact on API calls, mocked servers ?

By the way, this how I mock the server:

import supertest from 'supertest';
import app from '../src/app';

const request = supertest(app.callback());

await request
        .post('/api/v1/book')
        .set('Content-Type', 'application/json')
        .set('Accept', 'application/json')
        .send({
            businessDay,
            timeSlot,
            tableName,
        });
dubzzz commented 2 years ago

Impacts on mocks or API calls: not directly, or more precisely fast-check is fully framework agnostic.

BUT for Jest when using mocks, you must probably define a:

.beforeEach(() => {
  jest.resetAllMocks();
  // more precisely your beforeEach 
}) 

beforeEach is a function exposed onto properties created out of fast-check.

By the way, just in case, as it tend to be an issue I spotted many times when people reported issues with asyncProperty:

younes-io commented 2 years ago

@dubzzz , I already have beforeEach set up. This is the strange behaviour I have :

Test case OK: the await.request() works as expected

describe('Booking reservations', () => {
    it('should book a reservation [POST /reservation]', async () => {
        // Arrange & Act
        const businessDay = '16-03-2022';
        const timeSlot = '20:00';
        const tableName = 'Milano';
        const requestBody = {
            businessDay,
            timeSlot,
            tableName,
        };

        const bookedReservation = await bookAReservationMocked(requestBody);
        // Assert
        expect(bookedReservation.status).toEqual(200);
        expect(bookedReservation.body.booked).toEqual(true);
        expect(bookedReservation.body.slotStartHour).toEqual(timeSlot);
        expect(bookedReservation.body.tableName).toEqual(tableName);
    });
});

Test case KO: this test case passes even though it MUST fail

import fc from 'fast-check';
import supertest from 'supertest';
import app from '../src/app';
import { prismaMock } from './mocks/prisma.mock';   // Mocks the Prisma ORM (mocking MongoDB calls)

const request = supertest(app.callback()); // mocks server

afterAll(async () => {
    await prismaMock.$disconnect();
});

const reservationAPI = async ({ businessDay, timeSlot, tableName }) =>
    await request
        .post('/api/v1/book')
        .set('Content-Type', 'application/json')
        .set('Accept', 'application/json')
        .send({
            businessDay,
            timeSlot,
            tableName,
        });

describe.only('WIP: Booking reservations : race conditions', () => {
    it('works', async () => {
        fc.assert(
            fc
                .asyncProperty(fc.scheduler(), async (s) => {
                    // Arrange
                    const businessDay = '16-03-2022';
                    const timeSlot = '20:00';
                    const tableName = 'Milano';
                    const reservation = {
                        id: 'randomId',
                        slotId: 'randomId',
                        slotStartHour: timeSlot,
                        tableId: 'randomId',
                        tableName,
                        businessDay,
                        booked: false,
                    };
                    prismaMock.reservation.findFirst.mockResolvedValue(
                        reservation,
                    );
                    prismaMock.reservation.update.mockImplementation(
                        s.scheduleFunction(async () => {
                            return {
                                ...reservation,
                                booked: true,
                            };
                        }) as any,
                    );

                    const scheduledRes = s.scheduleFunction(reservationAPI); //.then((res) => res);
                    await scheduledRes({ businessDay, timeSlot, tableName });

                    await s.waitOne();
                    // Assert
                    expect(s.report()).toBe(5);    // DOES NOT THROW AN ERROR
                    expect(s.count()).toBe(20);   // DOES NOT THROW AN ERROR
                })
                .beforeEach(async () => {
                    jest.resetAllMocks();
                }),
            { verbose: 2 },
        );
    });
});
younes-io commented 2 years ago

I made some slight changes, and I got a different outcome:

const reservationAPI = async ({ businessDay, timeSlot, tableName }) => {
    const res = await request
        .post('/api/v1/book')
        .set('Content-Type', 'application/json')
        .set('Accept', 'application/json')
        .send({
            businessDay,
            timeSlot,
            tableName,
        });
    return JSON.stringify(res);   // Added this
};

Then, removed the await to await scheduledRes({ businessDay, timeSlot, tableName });, and the result now is:

image

Conclusion:

dubzzz commented 2 years ago

If I put await : await scheduledRes({ businessDay, timeSlot, tableName }); => the tests always pass, even though it should fail.. Why ?

Based on the screen shot you attached, the query "reservationAPI(...)" resolved successfully - see status:resolved - but it resolved into a value including an error code 415. In other words call to reservationAPI returned a successful Promise - not throwing - but including a status code 415.

Meanwhile, I'm quite surprised that await scheduledRes(...) passes. In theory as scheduledRes is a scheduled called waiting the s.waitOne() to be called to end it should be blocking forever... In other words, the call should probably not be awaited as it may holds forever.

If I remove the await : scheduledRes({ businessDay, timeSlot, tableName });, I get a 415 status code => why I get this HTTP response, even though I made the same HTTP call outside of fs.asyncProperty and I had a 200 ?

Probably some mocks that are not properly cleaned or reset between two runs 🤔 Maybe you need to add another clear-like thing for supertest/prisma?

For instance, can a query made during a first iteration of fast-check cause troubles for a next query? I mean, you seem to mount a mocked db over mongo, the first execution of the property (its predicate) might alter it (it will definitely plays with it). You may want to clean the mongo between two executions of the predicate (with the beforeEach/afterEach function defined on property).

In other words, I am wondering if the problem is not just that we don't clean the cache accross runs making assumptions broken when we start the next run. A bit like something like:

const fakeCache = new Map();
test('should only add one key and be able to list it', () => {
  fc.assert(fc.property(fc.string(), fc.anything(), (key, value) => {
    fakeCache.set(key, value);
    expect(fakeCache.has(key)).toBe(true);
    expect(fakeCache.size).toBe(1);
  }))
})

Would be fixed with:

const fakeCache = new Map();
test('should only add one key and be able to list it', () => {
  fc.assert(fc.property(fc.string(), fc.anything(), (key, value) => {
    fakeCache.set(key, value);
    expect(fakeCache.has(key)).toBe(true);
    expect(fakeCache.size).toBe(1);
  }).beforeEach(() => fakeCache.clear()));
})
younes-io commented 2 years ago

Thank you @dubzzz for the clarifications.

Although when I added the below block, it didn't change the outcome. Still having the 415 error, which I don't get when I make the call outside the fs.asyncProperty()

beforeEach(async () => {
                    request = supertest(app.callback());
                    await prismaMock.reservation.deleteMany();
                    jest.resetAllMocks();
                    jest.resetModules();
                }),

Besides, the 415 HTTP code means that the request content type / encoding is not supported. So, I don't see how any of the mocks could impact it; except the server mock, which in the beforeEach I override in every run...

Do you have any real-world projects that use fast-check to simulate race conditions on an API / webserver ?

Most of the examples in the documentation are React/front-end driven; I thought if I could find some real-world tests (besides the examples), it could help..

dubzzz commented 2 years ago

I don't know of any other open source examples using race condition detection provided. On my side I do use it every day at work but on a closed source project.

The issue, you reported, should be reproducible outside of fast-check. Actually as fast-check does not do any implicit magic around the mocks, the issue is probably reproducible outside of fast-check just by inlining the content of the predicate twice in the same test. It seems to be something not going well when called a second time.

Just to confirm that theory: does the test passes if numRuns:1 is passed to assert? If not, then there is possibility something wrong in how you use the scheduler but I was not able to spot it on my mobile yet.

dubzzz commented 2 years ago

Unfortunately so far I cannot really help more without an example I could run locally (codesandbox or small repo with the repro).

younes-io commented 2 years ago

Thank you @dubzzz

I have been debugging and I realized that the error causing the 415 is similar to https://github.com/jsdom/jsdom/issues/2060

So, I added these couple of lines at the top of my test file and now I have a 404 instead even when I add { numRuns : 1}

import * as iconv from 'iconv-lite';
iconv.encodingExists('utf8');

Please find the code here : https://github.com/younes-io/booking-manager-api/blob/feature/PBT/__tests__/reservations_concurrency.test.ts (it's all in the branch feature/PBT)

dubzzz commented 2 years ago

Thanks for the repro at https://github.com/younes-io/booking-manager-api/blob/feature/PBT/__tests__/reservations_concurrency.test.ts I'll try to have a look to it this week

dubzzz commented 2 years ago

You probably want to try with:

    it('works', async () => {
+++        await fc.assert(
---        fc.assert(
dubzzz commented 2 years ago

Seems to go better with something like:

import fc from 'fast-check';
import supertest from 'supertest';
import app from '../src/app';
import { prismaMock } from './mocks/prisma.mock';

const request = supertest(app.callback());

afterAll(async () => {
    await prismaMock.$disconnect();
});

describe('Booking reservations', () => {
    it('should book a reservation [POST /reservation]', async () => {
        await fc.assert(
            fc.asyncProperty(fc.scheduler(), async (s) => {
                const businessDay = '16-03-2022';
                const timeSlot = '20:00';
                const tableName = 'Milano';

                const reservation = {
                    id: 'randomId',
                    slotId: 'randomId',
                    slotStartHour: timeSlot,
                    tableId: 'randomId',
                    tableName,
                    businessDay,
                    booked: false,
                };
                prismaMock.reservation.findFirst.mockImplementation(
                    s.scheduleFunction(async function findFirst() {
                        return reservation;
                    }) as any,
                );
                prismaMock.reservation.update.mockImplementation(
                    s.scheduleFunction(async function update() {
                        return {
                            ...reservation,
                            booked: true,
                        };
                    }) as any,
                );

                const bookedReservationPromise = request
                    .post('/api/v1/reservation')
                    .set('Content-Type', 'application/json')
                    .set('Accept', 'application/json')
                    .send({
                        businessDay,
                        timeSlot,
                        tableName,
                    });

                await s.waitAll();
                const bookedReservation = await bookedReservationPromise;

                expect(bookedReservation.status).toEqual(200);
                expect(bookedReservation.body.booked).toEqual(true);
                expect(bookedReservation.body.slotStartHour).toEqual(timeSlot);
                expect(bookedReservation.body.tableName).toEqual(tableName);
            }),
            { timeout: 1000 },
        );
    });
});
  1. I removed the .beforeEach so far but they are probably partly needed as you possibly want not to alter next executions of the predicate with data mocked in the previous runs. Please note that .beforeEach of fast-check will not use data set at the level of beforeEach of Jest
  2. It seems that the test time-out... so I added an automatic timeout to mark it as failure in such case but I'll probably dig a littl bit more to get what is going wrong with it
dubzzz commented 2 years ago

@younes-io This one is working:

import fc from 'fast-check';
import supertest from 'supertest';
import app from '../src/app';
import { prismaMock } from './mocks/prisma.mock';

const request = supertest(app.callback());

afterAll(async () => {
    await prismaMock.$disconnect();
});

describe('Booking reservations', () => {
    it('should book a reservation [POST /reservation]', async () => {
        await fc.assert(
            fc.asyncProperty(fc.scheduler(), async (s) => {
                const businessDay = '16-03-2022';
                const timeSlot = '20:00';
                const tableName = 'Milano';

                const reservation = {
                    id: 'randomId',
                    slotId: 'randomId',
                    slotStartHour: timeSlot,
                    tableId: 'randomId',
                    tableName,
                    businessDay,
                    booked: false,
                };

                prismaMock.reservation.findFirst.mockReturnValue(
                    s.schedule(
                        Promise.resolve(reservation),
                        'findFirst',
                    ) as any,
                );
                prismaMock.reservation.update.mockReturnValue(
                    s.schedule(
                        Promise.resolve({ ...reservation, booked: true }),
                        'update',
                    ) as any,
                );

                const bookedReservationPromise = request
                    .post('/api/v1/reservation')
                    .set('Content-Type', 'application/json')
                    .set('Accept', 'application/json')
                    .send({
                        businessDay,
                        timeSlot,
                        tableName,
                    });

                await s.waitAll();
                const bookedReservation = await bookedReservationPromise;

                expect(bookedReservation.status).toEqual(200);
                expect(bookedReservation.body.booked).toEqual(true);
                expect(bookedReservation.body.slotStartHour).toEqual(timeSlot);
                expect(bookedReservation.body.tableName).toEqual(tableName);
            }),
        );
    });
});

Meanwhile, there is something strange with mockImplementation... I don't get why but it's as if the implementation was never called when using it or called later... As in your case you already now the values to be resolved the s.schedule is good enough at the moment.

younes-io commented 2 years ago

Thank you @dubzzz !

Actually, in my use case, I don't know the values to be resolved... I mean there is a logic, that's why I use mockImplementation

For instance, when I use this code, I get a timeout error :

prismaMock.reservation.findFirst.mockResolvedValue(reservation);
prismaMock.reservation.update.mockImplementation(
    s.scheduleFunction(async function update() {
        reservation.booked = true;
        return reservation;
    }) as any,
);

When I limit the timeout to 2000 for example, I get this; it seems as if the scheduler never resolves the promise, even though there is an await s.waitAll()

image

dubzzz commented 2 years ago

There is definitely something strange with prisma or supertest... I expect this test to pass. I mean, given that we want to play with the internals called to compute bookedReservation, we should be able to capture calls to the APIs otherwise it would never work...

import supertest from 'supertest';
import app from '../src/app';
import { prismaMock } from './mocks/prisma.mock';

const request = supertest(app.callback());

afterAll(async () => {
    await prismaMock.$disconnect();
});

describe('Booking reservations', () => {
    it('should book a reservation [POST /reservation]', async () => {
        const businessDay = '16-03-2022';
        const timeSlot = '20:00';
        const tableName = 'Milano';

        const reservation = {
            id: 'randomId',
            slotId: 'randomId',
            slotStartHour: timeSlot,
            tableId: 'randomId',
            tableName,
            businessDay,
            booked: false,
        };

        let calls = 0;

        prismaMock.reservation.findFirst.mockImplementation((() => {
            calls++;
            return reservation;
        }) as any);
        prismaMock.reservation.update.mockImplementation((() => {
            calls++;
            return { ...reservation, booked: true };
        }) as any);

        const bookedReservationPromise = request
            .post('/api/v1/reservation')
            .set('Content-Type', 'application/json')
            .set('Accept', 'application/json')
            .send({
                businessDay,
                timeSlot,
                tableName,
            });

        expect(calls).not.toBe(0); // <----------------------------------- here it fails with 0, apis have not been called
        const bookedReservation = await bookedReservationPromise;

        expect(bookedReservation.status).toEqual(200);
        expect(bookedReservation.body.booked).toEqual(true);
        expect(bookedReservation.body.slotStartHour).toEqual(timeSlot);
        expect(bookedReservation.body.tableName).toEqual(tableName);
    });
});

The only way to get the non zero value is to wait the result, but it is not an option. I mean if we wait the result they it means that the calls to findFirst and update already resolved so they have not passed thought the scheduler.

I don't know anything about the internals of those two libraries but the problem seems to come from how they work as there is no way to come between the resolution of the final promise and the utility calls used to build it.

dubzzz commented 2 years ago

The snippet below shows that there is no obvious way to have the calls done except resolving the api request:

import supertest from 'supertest';
import app from '../src/app';
import { prismaMock } from './mocks/prisma.mock';

const request = supertest(app.callback());

afterAll(async () => {
    await prismaMock.$disconnect();
});

describe('Booking reservations', () => {
    it('should book a reservation [POST /reservation]', async () => {
        const businessDay = '16-03-2022';
        const timeSlot = '20:00';
        const tableName = 'Milano';

        const reservation = {
            id: 'randomId',
            slotId: 'randomId',
            slotStartHour: timeSlot,
            tableId: 'randomId',
            tableName,
            businessDay,
            booked: false,
        };

        let calls = 0;

        prismaMock.reservation.findFirst.mockImplementation((() => {
            calls++;
            return reservation;
        }) as any);
        prismaMock.reservation.update.mockImplementation((() => {
            calls++;
            return { ...reservation, booked: true };
        }) as any);

        const bookedReservationPromise = request
            .post('/api/v1/reservation')
            .set('Content-Type', 'application/json')
            .set('Accept', 'application/json')
            .send({
                businessDay,
                timeSlot,
                tableName,
            });

        while (calls === 0) {
            await new Promise((r) => setTimeout(r, 100));
        }

        const bookedReservation = await bookedReservationPromise;

        expect(bookedReservation.status).toEqual(200);
        expect(bookedReservation.body.booked).toEqual(true);
        expect(bookedReservation.body.slotStartHour).toEqual(timeSlot);
        expect(bookedReservation.body.tableName).toEqual(tableName);
    });
});

In other words, fast-check cannot say: I want findFirst then update to compute bookedReservation as if the computation of bookedReservation has not be fully fulfilled so it does not see others.

dubzzz commented 2 years ago

Here is the trick to make it work: sadly we need to call then to trigger the request. In other words, we can make it work with the following:

describe('Booking reservations PBT', () => {
    it('should book a reservation [POST /reservation]', async () => {
        await fc.assert(
            fc.asyncProperty(fc.scheduler(), async (s) => {
                const businessDay = '16-03-2022';
                const timeSlot = '20:00';
                const tableName = 'Milano';

                const reservation = {
                    id: 'randomId',
                    slotId: 'randomId',
                    slotStartHour: timeSlot,
                    tableId: 'randomId',
                    tableName,
                    businessDay,
                    booked: false,
                };
                prismaMock.reservation.findFirst.mockImplementation(
                    s.scheduleFunction(async function findFirst() {
                        return reservation;
                    }) as any,
                );
                prismaMock.reservation.update.mockImplementation(
                    s.scheduleFunction(async function update() {
                        return {
                            ...reservation,
                            booked: true,
                        };
                    }) as any,
                );

                let resolveBookedReservation: (
                    v: supertest.Response | PromiseLike<supertest.Response>,
                ) => void;
                const bookedReservationPromise =
                    new Promise<supertest.Response>(
                        (resolve) => (resolveBookedReservation = resolve),
                    );
                request
                    .post('/api/v1/reservation')
                    .set('Content-Type', 'application/json')
                    .set('Accept', 'application/json')
                    .send({
                        businessDay,
                        timeSlot,
                        tableName,
                    })
                    .then((r) => resolveBookedReservation(r));

                while (s.count() === 0) {
                    await new Promise((r) => setTimeout(r, 100));
                }
                await s.waitAll();
                const bookedReservation = await bookedReservationPromise;

                expect(bookedReservation.status).toEqual(200);
                expect(bookedReservation.body.booked).toEqual(true);
                expect(bookedReservation.body.slotStartHour).toEqual(timeSlot);
                expect(bookedReservation.body.tableName).toEqual(tableName);
            }),
            { timeout: 1000, numRuns: 1 },
        );
    });
});

The while true on s.count() is purely there to wait enough before having the first call really issued by supertest...

dubzzz commented 2 years ago

Better but not perfect, replace:

                while (s.count() === 0) {
                    await new Promise((r) => setTimeout(r, 100));
                }
                await s.waitAll();

by:

                while (s.report().length !== 2 || s.count() !== 0) {
                    await new Promise((r) => setTimeout(r, 0));
                    await s.waitAll();
                }

Not having await new Promise((r) => setTimeout(r, 0)); make supertest never issuing the requests...


By the way no more need for timeout or numRuns, it seems to work with them not being specified too.

dubzzz commented 2 years ago

Now that I have a clearer idea of what caused the issue, I will certainly add another wait* onto the instance of scheduler. Something like: waitFor that would be able to wait for a promise and unlock potential dependencies it can rely on as they appear. In your case, it would be use to wrap the request.

younes-io commented 2 years ago

In other words, fast-check cannot say: I want findFirst then update to compute bookedReservation as if the computation of bookedReservation has not be fully fulfilled so it does not see others.

That's why I have used mockResolvedValue for the update function and the mockImplementation for the firstUpdate. I thought that with this, the scheduler will only schedule the firstUpdate because the update function would have resolved immediately.

Also, I was thinking about the scheduleSequence, but I' not sure how to use it in this case..

younes-io commented 2 years ago

Better but not perfect, replace:

                while (s.count() === 0) {
                    await new Promise((r) => setTimeout(r, 100));
                }
                await s.waitAll();

by:

                while (s.report().length !== 2 || s.count() !== 0) {
                    await new Promise((r) => setTimeout(r, 0));
                    await s.waitAll();
                }

Not having await new Promise((r) => setTimeout(r, 0)); make supertest never issuing the requests...

By the way no more need for timeout or numRuns, it seems to work with them not being specified too.

From my understanding, waitAll would do the job since it waits for all the scheduled tasks, right? But I guess in my case, the problem is that the scheduler does not wait for supertest to run the API call, right?

It's probably a lot to ask, but I would definitely be glad to have a diagram that describes how the scheduler functions and its inner workings..

Thank you @dubzzz ! Can't wait for the waitFor method!

dubzzz commented 2 years ago

From my understanding, waitAll would do the job since it waits for all the scheduled tasks, right? But I guess in my case, the problem is that the scheduler does not wait for supertest to run the API call, right?

More precisely the problem is how supertest beehaves. It does not trigger the API call if you don't explicitely call then either yourself or with an await. Additionally it does not triggers that one synchronously.

It's probably a lot to ask, but I would definitely be glad to have a diagram that describes how the scheduler functions and its inner workings..

Code is open source, the source of the scheduler can be found there for more details: https://github.com/dubzzz/fast-check/blob/main/src/arbitrary/_internals/implementations/SchedulerImplem.ts

younes-io commented 2 years ago

I noticed that this works :

prismaMock.reservation.update.mockImplementation(
                    s.scheduleFunction(async function update() {
                        return { ...reservation, booked: true };
                    }) as any,
                );

But this does not:

prismaMock.reservation.update.mockImplementation(
                    s.scheduleFunction(async function update() {
                        reservation.booked = true;
                        return reservation;
                    }) as any,
                );

Is it because of this line?

dubzzz commented 2 years ago

Next minor of fast-check will probably include a new helper called waitFor to better fit with the need you had for your test. The PR adding this new one is #2807 and can be tested via a simple npm i https://62379220dbdc7919b19c2f5e--dazzling-goodall-4f0e38.netlify.app/fast-check.tgz --save-dev.

The resulting code will be way simpler:

import fc from 'fast-check';
import supertest from 'supertest';
import app from '../src/app';
import { prismaMock } from './mocks/prisma.mock';

const request = supertest(app.callback());

afterAll(async () => {
    await prismaMock.$disconnect();
});

describe('Booking reservations PBT', () => {
    it('should book a reservation [POST /reservation]', async () => {
        await fc.assert(
            fc.asyncProperty(fc.scheduler(), async (s) => {
                const businessDay = '16-03-2022';
                const timeSlot = '20:00';
                const tableName = 'Milano';

                const reservation = {
                    id: 'randomId',
                    slotId: 'randomId',
                    slotStartHour: timeSlot,
                    tableId: 'randomId',
                    tableName,
                    businessDay,
                    booked: false,
                };
                prismaMock.reservation.findFirst.mockImplementation(
                    s.scheduleFunction(async function findFirst() {
                        return reservation;
                    }) as any,
                );
                prismaMock.reservation.update.mockImplementation(
                    s.scheduleFunction(async function update() {
                        return {
                            ...reservation,
                            booked: true,
                        };
                    }) as any,
                );

                const bookedReservation = await s.waitFor(
                    request
                        .post('/api/v1/reservation')
                        .set('Content-Type', 'application/json')
                        .set('Accept', 'application/json')
                        .send({
                            businessDay,
                            timeSlot,
                            tableName,
                        }),
                );

                expect(bookedReservation.status).toEqual(200);
                expect(bookedReservation.body.booked).toEqual(true);
                expect(bookedReservation.body.slotStartHour).toEqual(timeSlot);
                expect(bookedReservation.body.tableName).toEqual(tableName);
            }),
        );
    });
});
younes-io commented 2 years ago

@dubzzz : I was just wondering if you could explain the following behaviour:

The aim of the test below is to intercept concurrency cases. Basically, I want to fire to concurrent requests that would both compete to: 1- check if the reservation is available 2- book the reservation if available, otherwise reject

I am not sure if the code below simulates this. I think waitFor does not help simulating the concurrency / race situation between 2 requests. What do you think?

it('should handle race conditions', async () => {
        const businessDay = '16-03-2022';
        const timeSlot = '20:00';
        const tableName = 'Milano';
        // const customerName = 'Adam';

        await fc.assert(
            fc.asyncProperty(fc.scheduler(), async (s) => {
                const reservation = {
                    id: 'randomId',
                    slotId: 'randomId',
                    slotStartHour: timeSlot,
                    tableId: 'randomId',
                    tableName,
                    businessDay,
                    booked: false,
                    customerName: 'Adam',
                };
                prismaMock.reservation.findFirst.mockImplementation(
                    s.scheduleFunction(async function findFirst() {
                        if (!reservation.booked) return reservation;
                        return null;
                    }) as any,
                );

                prismaMock.reservation.update.mockImplementation(
                    s.scheduleFunction(async function update() {
                        reservation.booked = true;
                        return reservation;
                    }) as any,
                );

                const [bookedReservation1, bookedReservation2] =
                    await Promise.all([
                        s.waitFor(
                            request
                                .post('/api/v1/reservation')
                                .set('Content-Type', 'application/json')
                                .set('Accept', 'application/json')
                                .send({
                                    businessDay,
                                    timeSlot,
                                    tableName,
                                }),
                        ),
                        s.waitFor(
                            request
                                .post('/api/v1/reservation')
                                .set('Content-Type', 'application/json')
                                .set('Accept', 'application/json')
                                .send({
                                    businessDay,
                                    timeSlot,
                                    tableName,
                                }),
                        ),
                    ]);

                expect(bookedReservation1.status).not.toEqual(
                    bookedReservation2.status,
                );
                // expect(bookedReservation2.status).toBe(200);
                // expect(bookedReservation1.body.booked).toEqual(true);
                // expect(bookedReservation1.body.slotStartHour).toEqual(timeSlot);
                // expect(bookedReservation1.body.tableName).toEqual(tableName);
                // expect(bookedReservation1.body.customerName).toEqual(
                //     customerName,
                // );
            }),
        );
    });
dubzzz commented 2 years ago

Why not a waitAll(Promise.all(r1, r2))?

Otherwise (from my phone), the code seems ok 👌 What's the exact issue?

younes-io commented 2 years ago

Why not a waitAll(Promise.all(r1, r2))?

Otherwise (from my phone), the code seems ok 👌 What's the exact issue?

My code above does not simulate concurrency; it rather fires two requests and resolves them sequentially. That's not the setup I want to simulate. I would like to simulate the case where two requests are fired -almost- simultaneously.

For instance, I want to simulate this case: 1 - Request A (from User A) checks if a reservation X is valid and it finds out it is 2 - Request B (from User B) checks if a reservation X is valid and it finds out it is 3 - Request A books the reservation X => Now, User A has booked the reservation A 4 - Request B books the reservation X => Now, User B has booked the reservation A => PROBLEM !! Because, now, we have two users (A and B) who have been able to book the same reservation.

By the way, when I do a waitAll(Promise.all(r1,r2)), I get this (even with { numRuns: 1 }):

 thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."
dubzzz commented 2 years ago

~If you call Promise.all(r1,r2) without the waitFor and the scheduler do you have the same timeout error? In other words, is the timeout problem the same if ran outside of fast-check?~

Investigating what goes wrong... but I reproduced the issue and it seems as is fast-check was not waiting enough and just booked A then B but not in parallel. By adding some setTimeout in scheduler's code I get things working...

dubzzz commented 2 years ago

Here is an example based on supertest: https://github.com/dubzzz/fast-check/pull/2813

The key thing to make it work with supertest was to add a dedicated act function. Without it, queries only run one at a time. It seems to be related to internals within supertest but I was not able to find which ones. In the PR I define the exact same test but outside of supertest and there is no need for this act, everything works well out-of-the-box.

You can try running the examples against both the working and not working version to play with it if needed.

dubzzz commented 2 years ago

After double checking the example added for supertest, I confirm that the act-based solution + waitFor is the way to go for supertest. They answer to design choices inherent to superagent that make it harder to intercept but at the end it works 👍

Will probably release a minor of fast-check including waitFor in the coming days.

dubzzz commented 2 years ago

Closing the issue as there is no activity on it. Feel free to re-open it or open another one referencing this one if you believe there is still something to check.