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

fix(#324): runs xpath tests with the same timezone #323

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

latin-panda
Copy link
Collaborator

@latin-panda latin-panda commented Mar 4, 2025

Closes #324

I have verified this PR works in these browsers (latest versions):

  • Chrome
  • Firefox
  • Safari (macOS)
  • Safari (iOS)
  • Not applicable

What else has been done to verify this works as intended?

To reproduce the error:

  1. Change the machine time to March 1st at 23:30 Los Angeles timezone
  2. Run the test suite yarn workspace @getodk/xpath test
  3. See the tests failing like this:
Screenshot 2025-03-03 at 8 37 59 PM

Other things done:

After implementing the fix, I tried setting my machine in different timezones, ran the tests, and they passed. The different timezones have been passed to the "now" function correctly.

The offset is being appended to the localDateTimeString

I tried making a test about DTS, but it's hard to trick reliably the test to take a different DTS of the same timezone. However, I added some debugging code in localDateTimeString function, and I can see the offset switching to/from DTS for the same timezone.

// The process.env.TZ value is America/New_York
// Current date is March 3rd

console.log(dateTime.offset); // Prints "-05:00"
const dstDate = Temporal.ZonedDateTime.from("2025-07-01T23:30:00[America/New_York]");
console.log(dstDate.offset); // Prints "-04:00"

Why is this the best possible solution? Were any other approaches considered?

This ensures the tests are run in the same timezone by setting it in the beforeEach hook using the useFakeTimers utility. However, this doesn't mock the DST, which has been challenging to tick the system timer.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

N/A

Do we need any specific form for testing your changes? If so, please attach one.

I don't think so

What's changed

  • Adds a global beforeEach and AfterEach hook to set the tests' timezone
  • Adds a new optional environment variable for the tests' locale

Copy link

changeset-bot bot commented Mar 4, 2025

⚠️ No Changeset found

Latest commit: 4ae1011

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@latin-panda latin-panda changed the title Fixes xpath date tests fix(#324): runs xpath tests with the same timezone Mar 4, 2025
@latin-panda latin-panda marked this pull request as ready for review March 4, 2025 04:49
*
* @example 'en-US' // American English
*/
// eslint-disable-next-line no-var
var IMPLEMENTATION: string | undefined;
Copy link
Collaborator Author

@latin-panda latin-panda Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find the usage of this IMPLEMENTATION. I wanted to add a JSDoc description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added in this commit . It's unclear what it is for. I'll leave it there for now and ask Trevor later

@latin-panda
Copy link
Collaborator Author

@eyelidlessness When you have a chance, can you please review it? Thank you!

@@ -21,6 +21,7 @@ defaults:

env:
TZ: 'America/Phoenix'
LOCALE_ID: 'en-US'
Copy link
Collaborator Author

@latin-panda latin-panda Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added for completeness and to clarify that the tests expect the date format in this locale.
When switching to another locale (e.g., es-AR or es-MX), some tests fail because they expect a different date format.

With es-MX the now() returns a 1-1-1970 date, and tests using today() fail.
With es-AR the now() returns the current date correctly. But other tests fails, where tomorrow is set in en-US standard 2025-03-05

 FAIL  test/xforms/date-comparison.test.ts > date comparison > tomorrow > should be greater than or equal to today()
AssertionError: expected false to deeply equal true

- Expected
+ Received

- true
+ false

Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the root cause of #324 then that the test was written with the assumption that the timezone was Arizona time and because that wasn't set uniformly the device time was being used in some cases?

// eslint-disable-next-line no-var
var TZ: string | undefined;
// eslint-disable-next-line no-var
var LOCALE_ID: string | undefined;
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go above the declaration, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! 🤦🏽‍♀️
Fixed

@latin-panda latin-panda force-pushed the fixes-xpath-date-tests branch from f37c289 to 08f8a05 Compare March 26, 2025 14:43
@latin-panda
Copy link
Collaborator Author

Was the root cause of #324 then that the test was written with the assumption that the timezone was Arizona time, and because that wasn't set uniformly the device time was being used in some cases?

@lognaturel, yes, that's what I understand.

The default timezone (Arizona) was added in this commit, with this JSDoc

TODO: Many integration tests concerned with datetimes currently expect a
fixed time zone, for hard-coded values. The time zone was also chosen
specifically because it does not (er, did not then, nor does presently at
time of writing) observe daylight saving time or any other periodic
change in its UTC offset.

This PR ensures the timezone is applied to all tests by introducing a global beforeEach hook and instructs the test runner to use a fake timezone (default to Arizona) and fake locale (default to en-US). These are reverted to use real machine timezone and locale in a global afterEach hook.

@latin-panda latin-panda requested a review from lognaturel March 26, 2025 15:07
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.

XPath's date tests fail when the machine's time is close to midnight
2 participants