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 3 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: f37c289

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.

@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

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
1 participant