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.

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 Node.js 18.x to workflows #6599

Merged
merged 3 commits into from
Dec 2, 2022
Merged

Conversation

sigv
Copy link
Contributor

@sigv sigv commented Nov 28, 2022

refs: #6489

Description

Node.js 14.x is retained as the default, just as it was before, considering it is Maintenance LTS set to EOL next year (2023-04-30).

This change adds Node.js 18.x in all the places where workflows previously were building with Node.js 16.x - either as addition to it where multiple version are provided, or as an upgrade where only one version is used.

This finishes up the bulk of #6489, however manual testing of compatibility should be conducted further before that.

Security Considerations

N/A. This is a change to test scenarios, to support the Current LTS of Node.js.

Documentation Considerations

Backwards compatibility should not be affected. As result of manual testing in #6489, it may be decided to inform users that Node.js 18.x is now officially supported. Upgrade of Node.js versions is breaking for compiled blobs, in which case users need to rebuild project when upgrading from Node.js 16.x to 18.x, similar to how they would do when upgrading from 14.x to 16.x.

Testing Considerations

The test suite is expanded to support Node.js 18.x. Additional manual testing is out of scope for this PR and can be evaluated/discussed in #6489.

@sigv sigv mentioned this pull request Nov 28, 2022
@sigv
Copy link
Contributor Author

sigv commented Nov 28, 2022

Be advised, Mergify should be adjusted to pick up on 18.x tests, and they should be also marked as "Expected"/"Required".

@sigv
Copy link
Contributor Author

sigv commented Nov 28, 2022

packages/solo yarn test invocation seems to have timed out after a TypeError. 🤔

@sigv
Copy link
Contributor Author

sigv commented Nov 28, 2022

Running locally with Node.js v18.12.1, I seem to get test execution in packages/solo:

yarn run v1.22.19
$ ava

# connecting
Loading slog sender modules: @agoric/telemetry/src/flight-recorder.js
Loading slog sender modules: @agoric/telemetry/src/flight-recorder.js
2022-11-28T17:43:41.521Z launch-chain: Expected SwingSet kernel statistic cosmic_swingset_inbound_queue_length not found
2022-11-28T17:43:41.522Z launch-chain: Expected SwingSet kernel statistic cosmic_swingset_inbound_queue_add not found
2022-11-28T17:43:41.522Z launch-chain: Expected SwingSet kernel statistic cosmic_swingset_inbound_queue_remove not found
agoric: deploy: running /root/agoric-sdk/packages/wallet/api/deploy.js

SES_UNHANDLED_REJECTION: (TypeError#1)
TypeError#1: Cannot deliver getBridge to target; typeof target is undefined

CapTP test fixture exception: (RemoteError(error:captp:unknown#20001)#1)
RemoteError(error:captp:unknown#20001)#1: board does not have id: (a string)
  at WebSocket.<anonymous> (packages/solo/test/captp-fixture.js:60:11)
  at WebSocket.emit (node:events:513:28)
  at Receiver.emit (node:events:513:28)
  at Socket.emit (node:events:513:28)

CapTP test fixture exception: (RemoteRangeError(error:captp:unknown#20002)#2)
RemoteRangeError(error:captp:unknown#20002)#2: id is probably a typo, cannot verify CRC: (a string)
  at WebSocket.<anonymous> (packages/solo/test/captp-fixture.js:60:11)
  at WebSocket.emit (node:events:513:28)
  at Receiver.emit (node:events:513:28)
  at Socket.emit (node:events:513:28)

  ✔ home.board (7.7s)
  ✔ home.wallet - transfer funds to the feePurse (4.4s)
  ✔ home.wallet - receive zoe invite (17.4s)
    ℹ {
        contractRoot: '/root/agoric-sdk/packages/zoe/src/contracts/automaticRefund.js',
      }
  ✔ home.wallet - central issuer setup (2.6s)
  ✔ home.localTimerService makeNotifier (1.1s)
  ✔ home.localTimerService makeRepeater (895ms)
2022-11-28T17:47:58.091Z shutdown: Shutting down cleanly...
2022-11-28T17:47:58.092Z shutdown: Process terminated!
  ─

  6 tests passed
Done in 427.42s.

@sigv
Copy link
Contributor Author

sigv commented Nov 29, 2022

Will try to understand what could be different between the CI worker and my local installation, resulting in packages/solo hanging..

@sigv
Copy link
Contributor Author

sigv commented Nov 29, 2022

EBADF: bad file descriptor, read during build (14.x) when rebasing on top of latest master patches. Still haven't found a way to repro this locally.

@sigv
Copy link
Contributor Author

sigv commented Nov 29, 2022

It looks like test-home invoking the CLI does not return back to the Ava testcases. Must not believe the chain is ready for accepting work.

@sigv
Copy link
Contributor Author

sigv commented Nov 29, 2022

Bug in @endo/captp doesn't feel likely.. but maybe something with makeCapTP in particular? Still doesn't explain why this would cause an issue on the CI and not locally.

@sigv
Copy link
Contributor Author

sigv commented Nov 29, 2022

Could someone check that the runner ubuntu-latest actually has all packages up-to-date?
My local tests are on a fully updated Ubuntu 22.04.1 LTS (Jammy).

@mhofman
Copy link
Member

mhofman commented Nov 29, 2022

I can't look at this this week, maybe @michaelfig can lend a hand?

@sigv
Copy link
Contributor Author

sigv commented Nov 29, 2022

Running that test on a clean Ubuntu 20.04 (Focal) image deployed in Azure on D8s_v5 (8 vCPUs, 32G RAM) seems to work. This feels more and more like a runner-specific issue, but I am not sure why this one package's test would be affected. 😐

Here is the minimal scenario:

curl -fsSL https://deb.nodesource.com/gpgkey/nodesource.gpg.key | gpg --dearmor | sudo tee /etc/apt/trusted.gpg.d/nodesource.gpg >/dev/null
echo "deb [signed-by=/etc/apt/trusted.gpg.d/nodesource.gpg] https://deb.nodesource.com/node_18.x $(lsb_release -s -c) main" | sudo tee /etc/apt/sources.list.d/nodesource.list >/dev/null

sudo apt update
sudo apt --yes dist-upgrade
sudo apt --yes install chromium-browser gcc g++ jq make nodejs perl

sudo corepack enable yarn

sudo wget -O /usr/local/go1.19.3.linux-amd64.tar.gz https://go.dev/dl/go1.19.3.linux-amd64.tar.gz
sudo tar -C /usr/local -xf /usr/local/go1.19.3.linux-amd64.tar.gz
eval $(echo 'export PATH=$PATH:/usr/local/go/bin' | sudo tee /etc/profile.d/golang.sh)

go version
# go version go1.19.3 linux/amd64
node --version
# v18.12.1
yarn --version
# 1.22.19

git clone -b maint/test-node-18.x https://github.com/sigv/agoric-sdk.git
cd agoric-sdk

yarn install
yarn build

cd packages/solo
yarn test

# 6 tests passed
# Done in 218.96s.

@sigv sigv force-pushed the maint/test-node-18.x branch 2 times, most recently from dda6cca to 21072b8 Compare November 30, 2022 16:58
@sigv sigv marked this pull request as draft November 30, 2022 16:59
@sigv
Copy link
Contributor Author

sigv commented Nov 30, 2022

Slowly learning more about how this is interconnected. Right now it seems the Websocket connection in packages/solo/test/captp-fixture.js keep failing and looping - resulting in the observable mix of dots (.) and some os. On Node 14.x and 16.x it's a consistent line of os - no waiting on connection.

Latest run does imply that the service itself comes online - as signaled by "Deployed Wallet!" - just that the test scenario never kicks off.


I also noticed that on Node 14.x the log output says

CapTP test fixture exception: (RemoteError(error:captp:http://localhost:7999#20001)#1)
RemoteError(error:captp:http://localhost:7999#20001)#1: board does not have id: (a string)
  at WebSocket.<anonymous> (packages/solo/test/captp-fixture.js:60:11)
  at WebSocket.emit (events.js:400:28)
  at Receiver.emit (events.js:400:28)
  at Socket.emit (events.js:400:28)

however, on Node 16.x it does not list the origin and instead says "unknown" as

CapTP test fixture exception: (RemoteError(error:captp:unknown#20001)#1)
RemoteError(error:captp:unknown#20001)#1: board does not have id: (a string)
  at WebSocket.<anonymous> (packages/solo/test/captp-fixture.js:60:11)
  at WebSocket.emit (node:events:513:28)
  at Receiver.emit (node:events:513:28)
  at Socket.emit (node:events:513:28)

I am locally observing "unknown" on Node 18.x as well, but just seemed a bit strange.

@sigv
Copy link
Contributor Author

sigv commented Nov 30, 2022

Node.js 17.0.0 enables native DNS result priority, which can result in IPv6 being preferred in many scenarios. I wonder if the IPv6 stack is the issue for the CI runner!

Node.js 16.x tests report Open CapTP connection to ws://127.0.0.1:7999/private/captp...
while Node.js 18.x tests fail with connect ECONNREFUSED ::1:7999

I did not enable IPv6 stack on my test VMs, and when testing locally the Linux box was only hooked up to IPv4 too. The test runner must have a native IPv6 address and prefers to use it then!

@sigv sigv force-pushed the maint/test-node-18.x branch 3 times, most recently from 2b6f92f to a6fa12f Compare November 30, 2022 20:29
@sigv sigv marked this pull request as ready for review November 30, 2022 20:29
@sigv
Copy link
Contributor Author

sigv commented Nov 30, 2022

Sorry for the messy investigation and various rebases.

@mhofman, @michaelfig, this is ready for review and potential merge.

@mhofman mhofman added the force:integration Force integration tests to run on PR label Dec 2, 2022
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Thanks for investigative work you did on this PR. Once we revert to 16 on the deployment test I believe this can be merged.

Please also allow edits from maintainers for your branch so mergify can rebase as necessary before merging.

.github/workflows/deployment-test.yml Outdated Show resolved Hide resolved
@sigv sigv requested a review from mhofman December 2, 2022 16:37
@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Dec 2, 2022
With Node.js 17+ defaulting to IPv6, hosts that have native IPv6 stacks
might have resolved `localhost` to `::1`. (See: nodejs/node#39987) This
would generally be fine for most applications, but seems here listening
only happens on the IPv4 stack. This was not observed as an issue on
Node.js <= 16, because it prioritized IPv4 addresses historically.

This commit replaces `localhost` with `127.0.0.1` because there is a
`connections.json` which gets written the IPv4 address as well. At a
later time, the package should theoretically be re-checked, to better
handle (and use) the IPv6 stack.
sigv and others added 2 commits December 2, 2022 17:32
Node.js 14.x is retained as the default, just as it was before,
considering it is Maintenance LTS set to EOL next year (2023-04-30).

This change adds Node.js 18.x in all the places where workflows
previously were building with Node.js 16.x - either as addition
to it where multiple version are provided, or as an upgrade where
only one version is used.

This finishes up the bulk of Agoric#6489, however manual testing of
compatibility should be conducted further before that.
Ubuntu version on the agent running Deployment Test should be upgraded, so that it is compatible with Node 18. Reverting in the meantime.

Co-authored-by: Mathieu Hofman <86499+mhofman@users.noreply.github.com>
@sigv
Copy link
Contributor Author

sigv commented Dec 2, 2022

@mhofman, don't forget about marking the 18.x tests as "Expected"/"Required" so they are visible clearly in the GitHub UI, as well as adding them to Mergify checks!

@sigv
Copy link
Contributor Author

sigv commented Dec 2, 2022

And big thanks for the review! 🎉

@mhofman
Copy link
Member

mhofman commented Dec 2, 2022

don't forget about marking the 18.x tests as "Expected"/"Required"

Yup, planning on making them required, and removing the 14 ones from required

@mhofman
Copy link
Member

mhofman commented Dec 2, 2022

And big thanks for the review! 🎉

Thanks so much for your contribution.

@mergify mergify bot merged commit 1b3c99b into Agoric:master Dec 2, 2022
@sigv sigv deleted the maint/test-node-18.x branch December 2, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants