CI: Run package/date unit tests in different timezones #27552
Conversation
ockham
added
[Status] In Progress
|
Size Change: -278 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
add/timezones-matrix-for-date-functions-unit-test-ci
branch
from
d271c13 to
18fef83
Compare
|
Rebased, and pushed a few more commits. |
There was a problem hiding this comment.
Have you considered updating the test suite for dates package to run with different time zones instead?
There is https://jestjs.io/docs/en/api#describeeachtablename-fn-timeout that could help here.
|
@gziolo It's not possible to change timezone and locale during runtime in Node.js. One option would be to use |
Have you tried mocking You can also reset the state with: It should be possible to load the same package several times and initialize it differently.
The changes proposed to CI are complicated as well. Another downside is that contributors won't be able to run the same tests locally. |
|
@gziolo As @david-szabo97 said, our first choice would've been to change the timezone (and locale) through Jest so it'd be easier to run the tests locally (as part of a normal test run). However, this turned out to be very complicated, as there doesn't seem to be a lot of dedicated utilities to that end, so we chose the CI-based approach, as it's much more straight-forward, reasonably elegant, and provides a solid baseline of unit tests against regressions.
I haven't, but TBH, I don't think that is what we should be testing for, since it'd test at implementation level. The context for this PR is that the But the whole point of this PR is to guard against regressions when working on the implementation of To be clear, I'm not opposed to a Jest-level solution (as long as it's implementation-agnostic), but I think it'll require much more work, and I'd like us to start with at least some coverage. I think we can come back to this later to figure out something at Jest level. Doing this at CI level provides a solid backstop for now; if something is broken for these timezones, CI will turn red and prevent a merge, prompting developers to investigate. If they look into the broken action, they should see fairly quickly that their code fails for a given timezone or locale, and be able to simulate that test locally (basically by prepending |
Co-authored-by: Dávid Szabó <[email protected]>
|
With 4 timezones and 4 locales, we now have 16 new entries in our CI. That seems a bit excessive. Considering cutting back a bit -- e.g. I don't think we need to test that many locales -- en_US and ja_JP is probably enough. |
There was a problem hiding this comment.
LGTM! ✅ Thank you so much for working on this!
With 4 timezones and 4 locales, we now have 16 new entries in our CI. That seems a bit excessive. Considering cutting back a bit -- e.g. I don't think we need to test that many locales -- en_US and ja_JP is probably enough.
Isn't there a way to merge them into a single entry? 🤔
Now that the PR was reverted and we have the old implementation that uses gutenberg/packages/date/src/test/index.js Lines 87 to 91 in 18349c4 |
|
|
Thank you for reviewing/testing/approving!
There is a way to couple them, yes. But I prefer to keep those as independent 'factors' -- since there's a chance we might miss bugs if we limit ourselves to certain scenarios (e.g. only testing UTC with |
It guards us against regressions if we ever try to change the implementation of |
|
I'll go ahead and merge this per the rationale above, since I'd like some closure on the If this needs more discussion, we can do that here or in a follow-up issue or PR 🙂 |
ockham
deleted the
add/timezones-matrix-for-date-functions-unit-test-ci
branch
| - name: Running the tests | ||
| env: | ||
| TZ: ${{ matrix.timezone }} | ||
| LANG: ${{ matrix.locale }} |
There was a problem hiding this comment.
Is there a way to achieve the same thing without creating a big number of jobs? Maybe by hooking into jsdom or something? We have limited resources on the WP org github running and this is going to hurt :(
There was a problem hiding this comment.
The other "easy" alternative is to just do the same thing that is done here but using a loop after the end of the unit-test job (without creating new jobs).
There was a problem hiding this comment.
One more (easy) alternative that comes to mind: We could limit these tests to be only run if files in packages/date/ are touched 🤔
There was a problem hiding this comment.
We'd miss package upgrades and things like that and if we do include package-lock.json they'd be triggering very often.
There was a problem hiding this comment.
How about a bash file or npm script which these tests?
#!/bin/bash
timezones=(EST GMT CET)
locales=(en_US ja_JP)
for timezone in "${timezones[@]}"; do
for locale in "${locales[@]}"; do
( TZ=$timezone LANG=$locale npm run test-unit -- packages/date )
done
donenpm script would become horribly long:
"unit-test:date": "TZ=EST LANG=en_US npm run test-unit -- packages/date && TZ=CET LANG=en_US npm run test-unit -- packages/date ......."
Fixes #27500. See #27550 (comment) for more context.
Description
Run unit tests for the
@wordpress/datepackage in different timezones, and with different locales. This will be important to guard against regressions once we re-attempt migrating away frommoment.jsto another date/time package (such asdate-fns).How has this been tested?
Verify that the Unit Tests GH action goes green.
Types of changes
Increase test coverage.
Checklist: