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.

Sign up for GitHub

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

test: add a test to ensure the correctness of timezone upgrades #45299

Merged
merged 7 commits into from
Nov 10, 2022
Merged

test: add a test to ensure the correctness of timezone upgrades #45299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f2e5e3
test: add a test to ensure the correctness of timezone upgrades
RaisinTen Nov 3, 2022
f004140
fixup! test: address review comments and skip if no intl
RaisinTen Nov 3, 2022
8780765
fixup! test: intl -> Intl in skip message
RaisinTen Nov 4, 2022
693f9bc
fixup! test: add extra guards
RaisinTen Nov 4, 2022
69ec584
fixup! test: add link to canned icu directory path
RaisinTen Nov 4, 2022
c3ee434
fixup! test: explain difference between small-icu and deps/icu-small
RaisinTen Nov 6, 2022
dfa9a34
fixup! test: fix typo configured -> configure
RaisinTen Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/timezone-update.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ jobs:

- run: ./tools/update-timezone.mjs

- name: Update the expected timezone version in test
run: echo "${{ env.new_version }}" > test/fixtures/tz-version.txt

- name: Open Pull Request
uses: gr2m/create-or-update-pull-request-action@dc1726cbf4dd3ce766af4ec29cfb660e0125e8ee # Create a PR or update the Action's existing PR
env:
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/tz-version.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
2022e
28 changes: 28 additions & 0 deletions test/parallel/test-tz-version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

const common = require('../common');

if (!common.hasIntl) {
common.skip('missing Intl');
}

// Refs: https://github.com/nodejs/node/blob/1af63a90ca3a59ca05b3a12ad7dbea04008db7d9/configure.py#L1694-L1711
if (process.config.variables.icu_path !== 'deps/icu-small') {
// If Node.js is configured to use its built-in ICU, it uses a strict subset
// of ICU formed using `tools/icu/shrink-icu-src.py`, which is present in
// `deps/icu-small`. It is not the same as configuring the build with
// `./configure --with-intl=small-icu`. The latter only uses a subset of the
// locales, i.e., it uses the English locale, `root,en`, by default and other
// locales can also be specified using the `--with-icu-locales` option.
common.skip('not using the icu data file present in deps/icu-small/source/data/in/icudt##l.dat.bz2');
}

const fixtures = require('../common/fixtures');

// This test ensures the correctness of the automated timezone upgrade PRs.

const { strictEqual } = require('assert');
const { readFileSync } = require('fs');

const expectedVersion = readFileSync(fixtures.path('tz-version.txt'), 'utf8').trim();
strictEqual(process.versions.tz, expectedVersion);
Copy link
Member

Choose a reason for hiding this comment

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

We support lots of different ways of building Node.js and ICU, including pointing to external ICU source/data files, so this test is going to need guards to work properly if its going to be part of the regular test suite.

node/configure.py

Lines 549 to 590 in 1af63a9

intl_optgroup.add_argument('--with-intl',
action='store',
dest='with_intl',
default='full-icu',
choices=valid_intl_modes,
help='Intl mode (valid choices: {0}) [default: %(default)s]'.format(
', '.join(valid_intl_modes)))
intl_optgroup.add_argument('--without-intl',
action='store_const',
dest='with_intl',
const='none',
help='Disable Intl, same as --with-intl=none (disables inspector)')
intl_optgroup.add_argument('--with-icu-path',
action='store',
dest='with_icu_path',
help='Path to icu.gyp (ICU i18n, Chromium version only.)')
icu_default_locales='root,en'
intl_optgroup.add_argument('--with-icu-locales',
action='store',
dest='with_icu_locales',
default=icu_default_locales,
help='Comma-separated list of locales for "small-icu". "root" is assumed. '
'[default: %(default)s]')
intl_optgroup.add_argument('--with-icu-source',
action='store',
dest='with_icu_source',
help='Intl mode: optional local path to icu/ dir, or path/URL of '
'the icu4c source archive. '
'v%d.x or later recommended.' % icu_versions['minimum_icu'])
intl_optgroup.add_argument('--with-icu-default-data-dir',
action='store',
dest='with_icu_default_data_dir',
help='Path to the icuXXdt{lb}.dat file. If unspecified, ICU data will '
'only be read if the NODE_ICU_DATA environment variable or the '
'--icu-data-dir runtime argument is used. This option has effect '
'only when Node.js is built with --with-intl=small-icu.')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a guard, hope that's enough?