Skip to content

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

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

Merged
nodejs-github-bot merged 7 commits into from
Nov 10, 2022
Merged

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

Changes from 1 commit
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
Prev Previous commit
Next Next commit
fixup! test: explain difference between small-icu and deps/icu-small 
Asked for in
#45299 (comment).

Signed-off-by: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
@RaisinTen
RaisinTen committed Nov 6, 2022
commit c3ee43496199321faf2fe809c42970b9c9a55372
6 changes: 6 additions & 0 deletions test/parallel/test-tz-version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ if (!common.hasIntl) {

// 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
// `./configured --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');
}
Comment on lines +10 to +18

@aduh95 aduh95 Nov 4, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 RaisinTen Nov 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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 aduh95 Nov 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 RaisinTen Nov 6, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.


Expand Down