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

[RaisinTen]

Currently, there's no way to know if a timezone upgrade PR is correct without building and testing the change locally. This change provides a solution for that.

Tested in RaisinTen#4.

Signed-off-by: Darshan Sen raisinten@gmail.com

[nodejs-github-bot]

Review requested:

• @nodejs/actions

[aduh95]

I don't really understand what is the meaning of this test, I don't see how checking env.new_version ensures the correctness of the upgrade – I mean, we could as well look at the PR name and got the same result, right?

[RaisinTen]

I don't really understand what is the meaning of this test, I don't see how checking env.new_version ensures the correctness of the upgrade

The test verifies that the version of timezone in the node binary with the change is indeed the same as what the intended version is. There's no way for reviewers to tell if the update is correct. I guess that's the reason why @richardlau attempted to build and test one of the upgrades locally in - #44950 (comment). I also saw that you and @lpinca RSLGTM-ed #45289 instead of LGTM-ing it, so I feel that this would provide enough confidence among reviewers that the update is correct.

I mean, we could as well look at the PR name and got the same result, right?

We could get the version the PR intends to upgrade timezone to from that but there's no way to tell if the binary actually has the new correct version. I think that can only be done by checking the version of timezone by running the node binary with the change like I'm doing here.

[richardlau]

The test verifies that the version of timezone in the node binary with the change is indeed the same as what the intended version is. There's no way for reviewers to tell if the update is correct. I guess that's the reason why @richardlau attempted to build and test one of the upgrades locally in - #44950 (comment).

Yes, that is exactly why I attempted to build locally to verify the reported version got updated.

[richardlau]

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.')

[RaisinTen]

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

[aduh95]

So this would work only if node is built with --with-intl=small-icu? Shouldn't we also check that if the value is undefined, we want to also run the test?

[RaisinTen]

So this would work only if node is built with --with-intl=small-icu?

Looks like that's not true:

node/configure.py

Lines 1703 to 1711 in 7124476

	# We can use 'deps/icu-small' - pre-canned ICU *iff*
	# - canned_is_full AND
	# - with_icu_source is unset (i.e. no other ICU was specified)
	#
	# This is *roughly* equivalent to
	# $ configure --with-intl=full-icu --with-icu-source=deps/icu-small
	# .. Except that we avoid copying icu-small over to deps/icu.
	# In this default case, deps/icu is ignored, although make clean will
	# still harmlessly remove deps/icu.

i.e., it should work if --with-intl is set to full-icu (default case).

Shouldn't we also check that if the value is undefined, we want to also run the test?

I don't think that's necessary because icu_path can never be undefined for the cases we are interested in (the objective is to run the test iff the current build is using deps/icu-small/source/data/in/icudt##l.dat.bz2):

• icu_path is set here and icu_full_path is never undefined -

  node/configure.py

  Line 1761 in 7124476

  o['variables']['icu_path'] = icu_full_path

• And this line wouldn't run only if we hit a return statement between

  node/configure.py

  Line 1584 in 7124476

  def configure_intl(o):

  and this assignment. The possible returns are:

  • node/configure.py

    Line 1641 in 7124476

    return

    - this would happen only if we pass --with-icu-path, I'm avoiding this to exclude the case where the pointed gyp file attempts to build icu with a different icudt file, which would result in a different process.versions.tz value

  • node/configure.py

    Line 1646 in 7124476

    return # no Intl

    - this would happen only if we pass --with-intl=none or --without-intl, which is not being considered because process.versions.tz would be undefined in this case

  • node/configure.py

    Line 1682 in 7124476

    return

    - this would happen only if we pass --with-intl=system-icu which we are avoiding because the system icu is not in our control, so it might produce a different version for process.versions.tz

[aduh95]

On macOS, node installed through brew:

$ node -p '"Intl" in globalThis'
true
$ node -p 'process.config.variables.icu_path'            
undefined

I tested this with Node.js v14.x, v16.x, and v18.x. Maybe it's using system-icu though, I don't know if there's another way to check. In any case, this should probably be explained in a comment why checking for 'deps/icu-small' covers more than small ICU builds.

[RaisinTen]

Maybe it's using system-icu though,

That is correct.

I don't know if there's another way to check.

There is :)

> process.config.variables.icu_gyp_path
'tools/icu/icu-system.gyp'

It relies on the gyp file for the systemwide icu instead of tools/icu/icu-generic.gyp which is for the builtin icu.
Alternatively, otool -L /usr/local/opt/node/bin/node would print a set of shared objects the binary dynamically links against and you'll see the icu dylibs in that list:

	/usr/local/opt/icu4c/lib/libicui18n.71.dylib (compatibility version 71.0.0, current version 71.1.0)
	/usr/local/opt/icu4c/lib/libicuuc.71.dylib (compatibility version 71.0.0, current version 71.1.0)
	/usr/local/opt/icu4c/lib/libicudata.71.dylib (compatibility version 71.0.0, current version 71.1.0)

In any case, this should probably be explained in a comment why checking for 'deps/icu-small' covers more than small ICU builds.

Explained, PTAL.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47754/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/45299
✔  Done loading data for nodejs/node/pull/45299
----------------------------------- PR info ------------------------------------
Title      test: add a test to ensure the correctness of timezone upgrades (#45299)
Author     Darshan Sen  (@RaisinTen)
Branch     RaisinTen:test/add-a-test-to-ensure-the-correctness-of-timezone-upgrades -> nodejs:main
Labels     meta, author ready, needs-ci, commit-queue-squash
Commits    7
 - test: add a test to ensure the correctness of timezone upgrades
 - fixup! test: address review comments and skip if no intl
 - fixup! test: intl -> Intl in skip message
 - fixup! test: add extra guards
 - fixup! test: add link to canned icu directory path
 - fixup! test: explain difference between small-icu and deps/icu-small
 - fixup! test: fix typo configured -> configure
Committers 1
 - Darshan Sen 
PR-URL: https://github.com/nodejs/node/pull/45299
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45299
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 03 Nov 2022 12:39:18 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45299#pullrequestreview-1169591418
   ✖  This PR needs to wait 2 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-11-07T02:32:44Z: https://ci.nodejs.org/job/node-test-pull-request/47754/
- Querying data for job/node-test-pull-request/47754/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/3436007316

[nodejs-github-bot]

Landed in d55a7c3