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

Set consistent timezone for tests #209

Merged
merged 1 commit into from
Sep 18, 2023
Merged

Conversation

sawyerh
Copy link
Contributor

@sawyerh sawyerh commented Sep 18, 2023

Changes

  • Fix common issue where a test passes locally, but fails in the CI or on other engineers' machines, because of a difference in timezone

@sawyerh sawyerh requested review from lorenyu and a team September 18, 2023 17:31
@sawyerh sawyerh merged commit 5db0cea into main Sep 18, 2023
6 checks passed
@sawyerh sawyerh deleted the sawyerh/set-jest-timezone branch September 18, 2023 17:36
@lorenyu
Copy link
Contributor

lorenyu commented Sep 18, 2023

I don't think I fully understand the motivation behind this change. I would imagine that if we run into a situation where a test passes/fails due to timezone differences, that that would indicate a potential issue in either (a) the source code or (b) the testing code. So this change might actually hide those sorts of bugs by making it seem like everything is running in the same timezone. Are there examples of tests we actually want to have assumptions on the timezone?

@sawyerh
Copy link
Contributor Author

sawyerh commented Sep 18, 2023

@lorenyu That's a good point. I was imagining a page that shows a date/time in the current user's timezone, which could vary, so the source code wouldn't define a specific timezone. Happy to revert this if you think the mocking implementation in tests should be left up to project teams.

@lorenyu
Copy link
Contributor

lorenyu commented Sep 18, 2023

I was imagining a page that shows a date/time in the current user's timezone, which could vary

Thanks for sharing that example. For that use case, I would put that in the test itself. e.g. something like:

test("show claim adjudication date/time in user's current timezone on the claim status page") {
  claim.determination_date = datetime(some datetime specified in utc)
  set timezone to PT
  output_pt = render(claim)
  expect(output_pt).toBe("11am PT")
  set timezoen to ET
  output_et = render(claim)
  expect(output_et).toBe("2pm ET")
}

that's just pseudocode but basically if the specific functionality / test cares about timezone, then just set it in that test. all other tests should be agnostic to timezone, and if they fail for some reason that probably indicates a deeper issue we shouldn't paper over.

sawyerh added a commit that referenced this pull request Sep 18, 2023
sawyerh added a commit that referenced this pull request Sep 18, 2023
This reverts commit 5db0cea.

## Context for reviewers

Projects can mock timezone within the relevant tests. Mocking globally
could hide bugs. See convo on #209.
@sawyerh
Copy link
Contributor Author

sawyerh commented Oct 24, 2023

I ran into an issue again where a globally defined timezone would have been helpful. The root of the issue, I think, is that process.env.TZ is actually cached, so setting it within an individual test doesn't guarantee that the timezone you set will be what's used. Here's a related issue about this: nodejs/node#3449

The TZ environment variable is cached by v8 for a small bit after the first time it is accessed (here when calling new Date()). It should update after a minute or so (not sure of the exact timeline in which the cache is read again)

From one of the Node.js contributors:

Should be set before the process runs, because it affects much of the initialization code. Probably should be set system wide, or at least for the user.

Here's the test I was trying to write:

it('reuses the token until it expires', async () => {
      // Set system to non-UTC just to ensure our implementation is taking timezones into account:
      process.env.TZ = 'America/Los_Angeles'

      jest.useFakeTimers({
        // Mock current time, in PT time:
        now: new Date('2043-10-30 19:00:00.000000'),
      })
      const spy = jest.spyOn(axios, 'post').mockResolvedValue({
        data: {
          data: {
            token: 'mock-token',
            // One hour in the future, but in UTC time
            expiration: '2043-10-31 02:00:00.000000',
          },
        },
      })

      const persistence = new PersistenceAPI({ baseUrl: MOCK_BASE_URL })

      // First token request results in an API call:
      await persistence.getToken()
      expect(spy).toHaveBeenCalledTimes(1)

      // Still only 1 API call after the second request:
      await persistence.getToken()
      expect(spy).toHaveBeenCalledTimes(1)

      // New API call after the token gets within 5 minutes of expiring:
      jest.setSystemTime(new Date('2043-10-30 19:55:00.000000'))
      await persistence.getToken()
      expect(spy).toHaveBeenCalledTimes(2)
    })

@lorenyu
Copy link
Contributor

lorenyu commented Oct 25, 2023

Interesting. How about this https://www.npmjs.com/package/timezone-mock

Looks like it mocks the time zone by replacing Date constructor

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

Successfully merging this pull request may close these issues.

3 participants