Thanks @iknowcss. I'm reticent to put things in here / change things as a result of the internals of how other frameworks behave. That being said, Sinon/Jest/other frameworks that use Sinon are quite common, and likely to cause issues for others. So if the change required to support them is maintainable etc. then i'm certainly open to discussion.

@mefellows yeah, I can definitely see where you're coming from. Practically speaking I'm not sure what other options we have. At the moment Pact fails without warning or any indication of the cause of the issue.

The only other way I can think to do this is to use a library which will wait for a condition (like the Pact server being up and running) rather than use setTimeout. And I'm sure that library will basically do this exact thing under the covers anyway.

What use case do you have for useFakeTimers in a Pact test? I'm asking because my gut feel is that fake timers would belong to a layer better tested by pure unit tests.

Hey @TimothyJones, that's a fair question. Here's a simplified version of the function we're testing. In short, there is an optional parameter checkpoint which takes a timestamp or defaults to Date.now().

export async function retrieveCompanyBookings(abn, checkpoint) {
  const { fromTimestamp, toTimestamp } = calculateDateRange(checkpoint || Date.now());
  const requestBody = { abn, fromTimestamp, toTimestamp };

  try {
    const response = await apiGatewayClient.post(COMPANY_BOOKINGS_URL, {requestBody});
    return {
      checkpoint: response.body.checkpoint,
      product: transformBookingData(response.body.product)
    }
  } catch (e) {
    // Error handling logic
  }
}

To your point, we could test the defaulting logic for checkpoint in a unit test, and I'm happy to do that. Something I am afraid of, though, is that someone else might get caught up on this and not have the time/patience/knowledge to dig into pact-js and then pact-node to realize that sinon.usesFakeTimers() is causing Pact to hang.

Here is a simplified version of how we use sinon.useFakeTimers() to stub Date.now() in our test:

const nowTimestamp = new Date('Feb 07 2018 12:34:56.789 GMT+1100').getTime();
let clock;

before(async () => {
  clock = sinon.useFakeTimers(nowTimestamp);
  await provider.setup();
});

it('retrieves products', async () => {
  await provider.addInteraction({
    state: 'default',
    uponReceiving: 'a request for products',
    withRequest: {
      method: 'POST',
      path: '/base/path/fake/url',
      headers: { 'Content-Type': 'application/json' },
      body: {
        abn,
        fromTimestamp: 1517880896000,
        toTimestamp: 1549503296000
      }
    },
    willRespondWith: {
      headers: { 'Content-Type': 'application/json' },
      status: 200,
      body: {
        product: [
          { pnr: 'MV3VX8' },
          { pnr: 'TSH8SC' }
        ],
        checkpoint: new Date('Mar 10 2018 12:00:01 GMT+1100').getTime()
      }
    }
  });


  const result = await expect(retrieveCompanyBookings('12345678901')).to.be.fulfilled;
  
  await provider.verify();
  expect(result).to.deep.eql({
    product: [
      { pnr: 'MV3VX8' },
      { pnr: 'TSH8SC' }
    ],
    checkpoint: new Date('Mar 10 2018 12:00:01 GMT+1100').getTime(),
    channels: ['foo', 'bar']
  });
});

after(async () => {
  clock.restore();
  await provider.finalize();
});

I believe my problem would be solved by moving the sinon.useFakeTimers() to after the point where I add the interaction. Or by not testing this logic using pact. But again I worry about others being affected by this and not understanding why.

I think your code seems like a reasonable place to be using Date.now().

If it really is a one line change as in #112, and doesn't impact anything else, I'm for this.

Are there other time mocking approaches that we would also fail with?

Whatever we do here, I think it's worth a documentation change, too - if pact-node doesn't work if setTimeout is mocked, we should document that. If it ignores mocking (as in your PR), I think we should document that, too.

Good catch!

To my knowledge any time mocking that overwrites setTimeout would affect us. Any other type of time mocking would not affect this project.

Would you like me to put in some documentation in my PR?

I'm not sure I understand the issue here and why you need fake timers at all. If you're using your pact tests to do both unit testing and contract testing, you're going to have a bad time when it comes to the minutiae of your business logic, hence the problem you seem to be hitting right now since sinon is meant to be used for unit testing, testing a single function's inputs and outputs.

My suggestion is that leave the contract testing to test properties, value types, etc, but without looking too deep at the value it's returning since that kind of detail is better represented in a unit test.

Hi @mboudreau, yeah, you're right that I can solve this particular problem without fake timers. However, I'm not sure it's safe to assume that Pact will be done outside of the context of unit testing. The Pact Best Practices explicitly state "Use Pact for isolated (unit) tests to test the class(es) that will be responsible for making the HTTP calls from your Consumer", which is what we are talking about here.

There's a high likelyhood that Pact users will have some mocking library in the mix. If they choose to mock the clock (as I did), they're going to be burned by the same problem. The dangerous thing is that, when that happens, they won't understand that useFakeTimers() is the cause of their grief. Given this, if we want to have a good developer experience, we really only have 2 options:

1. Detect for clock mocking and tell the user not to do that (which would require us to grab references to the global clock functions to make the comparison, plus logic to check every time we call one of these dangerous functions and then print to console) or
2. Guard against the problem: reference the clock functions at init and use those references instead of the global mutable ones

I 100% agree with your logic about not changing globals - but I do think that it's good for Pact to play nicely with other common testing frameworks (sinon is reasonably popular).

We might not agree with the philosophy of sinon, but I think it's best for Pact adoption if Pact "just works" with common testing frameworks.

Since a call to useFakeTimers() would always come after pact initialises, keeping our own reference would solve the problem - without any clear downsides (except needing to remember not to use the global directly, but that's easily handled with a lint rule).

I thought the usecase of an API request including a field calculated using the client time was reasonable, and sinon.useFakeTimers() was a reasonable way to write a pact test for that request (if you want to use sinon). So, if we don't make any code changes, I think it's worth at least adding a note to the documentation that pact will hang if you're using sinon.useFakeTimers(), if only to prevent people spending time digging to find the cause.

But from the perspective of increasing pact adoption, I think the one-line fix in #112 (plus a lint rule to prevent accidentally using the global directly) would be a better way to solve this.

If we can play nicely with Sinon, surely we should?

@TimothyJones I'm not convinced of that argument. Just because a semi-popular mocking library decides to implement something that breaks the world, I fail to see how that's our responsibility to support that. We do play nicely with Sinon for literally everything else, except something that changes globals, which is kind of the point it's trying to make here.

There are better ways to achieve this without a singular scope. By default Sinon replaces the global target, but there's no reason to be a lot more specific in it's usage as to not affect any libraries that are required.

Furthermore, even if we add global.setTimeout to our codebase, it still doesn't prevent the developer from overriding the globals before the require happens, something that I do quite often by clearing out the require cache and re-requiring the file to completely start anew properly and not have tests affect each other's state.

If the developer is trying to test out their own timers, they can do that within their own codebase without affecting the globals fairly easily. If it's not our library that will break, it will be another.

Further to this, I wondered if we could detect that the clock had been replaced and at least throw a helpful error - even if we could only detect sinon's clock replacement.

I looked through the sinon docs and source, and couldn't find anything directly helpful. I might open an issue with them.