-
Notifications
You must be signed in to change notification settings - Fork 13
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
base: main
Are you sure you want to change the base?
Conversation
|
* | ||
* @example 'en-US' // American English | ||
*/ | ||
// eslint-disable-next-line no-var | ||
var IMPLEMENTATION: string | undefined; |
There was a problem hiding this comment.
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.
@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' |
There was a problem hiding this comment.
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
Closes #324
I have verified this PR works in these browsers (latest versions):
What else has been done to verify this works as intended?
To reproduce the error:
yarn workspace @getodk/xpath test
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.
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 theuseFakeTimers
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