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

Attempt to migrate to Node20 #679

Merged
merged 6 commits into from May 11, 2024

Conversation

mikaelarguedas
Copy link
Contributor

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
…/parser@7.8.0'

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas mikaelarguedas requested review from emersonknapp and a team as code owners May 10, 2024 11:10
@mikaelarguedas mikaelarguedas requested review from gbiggs and removed request for a team May 10, 2024 11:10
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@christophebedard
Copy link
Member

Thanks for the PR! This looks great to me! Could you just update the version mentioned here to v20?

We are currently using Node.js v16.

@christophebedard
Copy link
Member

christophebedard commented May 10, 2024

You'll also need to update to node-version: "20.x" under actions/setup-node everywhere.

@christophebedard
Copy link
Member

christophebedard commented May 10, 2024

I get test failures when running the tests locally, there's no obvious cause 🤔

Click to expand
 FAIL  __test__/main.test.ts
  basic workflow tests
    ✕ run Linux workflow (10 ms)
    ✕ run Windows workflow (4 ms)
    ✓ run macOS workflow (2 ms)
  required-ros-distributions/noetic workflow tests
    ✕ run Linux workflow (2 ms)
    ✕ run Windows workflow
    ✓ run macOS workflow
  validate distribution test
    ✓ test valid
    ✓ test not valid (1 ms)

  ● basic workflow tests › run Linux workflow

    expect(received).resolves.not.toThrow()

    Received promise rejected instead of resolved
    Rejected to value: [TypeError: The "path" argument must be of type string. Received undefined]

      18 |
      19 |      it("run Linux workflow", async () => {
    > 20 |              await expect(linux.runLinux()).resolves.not.toThrow();
         |                    ^
      21 |      });
      22 |
      23 |      it("run Windows workflow", async () => {

      at expect (node_modules/expect/build/index.js:113:15)
      at __test__/main.test.ts:20:9
      at __test__/main.test.ts:31:71
      at Object.<anonymous>.__awaiter (__test__/main.test.ts:27:12)
      at Object.<anonymous> (__test__/main.test.ts:19:38)

  ● basic workflow tests › run Windows workflow

    expect(received).resolves.not.toThrow()

    Received promise rejected instead of resolved
    Rejected to value: [AssertionError: Expected RUNNER_TOOL_CACHE to be defined]

      22 |
      23 |      it("run Windows workflow", async () => {
    > 24 |              await expect(windows.runWindows()).resolves.not.toThrow();
         |                    ^
      25 |      });
      26 |
      27 |      it("run macOS workflow", async () => {

      at expect (node_modules/expect/build/index.js:113:15)
      at __test__/main.test.ts:24:9
      at __test__/main.test.ts:31:71
      at Object.<anonymous>.__awaiter (__test__/main.test.ts:27:12)
      at Object.<anonymous> (__test__/main.test.ts:23:40)

  ● required-ros-distributions/noetic workflow tests › run Linux workflow

    expect(received).resolves.not.toThrow()

    Received promise rejected instead of resolved
    Rejected to value: [TypeError: The "path" argument must be of type string. Received undefined]

      41 |
      42 |      it("run Linux workflow", async () => {
    > 43 |              await expect(linux.runLinux()).resolves.not.toThrow();
         |                    ^
      44 |      });
      45 |
      46 |      it("run Windows workflow", async () => {

      at expect (node_modules/expect/build/index.js:113:15)
      at __test__/main.test.ts:43:9
      at __test__/main.test.ts:31:71
      at Object.<anonymous>.__awaiter (__test__/main.test.ts:27:12)
      at Object.<anonymous> (__test__/main.test.ts:42:38)

  ● required-ros-distributions/noetic workflow tests › run Windows workflow

    expect(received).resolves.not.toThrow()

    Received promise rejected instead of resolved
    Rejected to value: [AssertionError: Expected RUNNER_TOOL_CACHE to be defined]

      45 |
      46 |      it("run Windows workflow", async () => {
    > 47 |              await expect(windows.runWindows()).resolves.not.toThrow();
         |                    ^
      48 |      });
      49 |
      50 |      it("run macOS workflow", async () => {

      at expect (node_modules/expect/build/index.js:113:15)
      at __test__/main.test.ts:47:9
      at __test__/main.test.ts:31:71
      at Object.<anonymous>.__awaiter (__test__/main.test.ts:27:12)
      at Object.<anonymous> (__test__/main.test.ts:46:40)

-----------------------|---------|----------|---------|---------|-------------------------------
File                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s             
-----------------------|---------|----------|---------|---------|-------------------------------
All files              |   60.89 |    26.66 |   56.66 |   60.89 |                               
 src                   |   55.17 |    26.66 |   56.25 |   55.17 |                               
  setup-ros-linux.ts   |   55.31 |        0 |      60 |   55.31 | 63-64,102-103,119-151,166-189 
  setup-ros-osx.ts     |     100 |      100 |     100 |     100 |                               
  setup-ros-windows.ts |   37.93 |        0 |      50 |   37.93 | 24-72,87                      
  utils.ts             |   53.57 |    57.14 |      50 |   53.57 | 28-40,68-80                   
 src/package_manager   |   71.42 |    26.66 |   57.14 |   71.42 |                               
  apt.ts               |   57.14 |        0 |   33.33 |   57.14 | 114-123                       
  brew.ts              |     100 |      100 |     100 |     100 |                               
  chocolatey.ts        |   52.63 |        0 |       0 |   52.63 | 40,52-59,68,80-88             
  pip.ts               |   84.21 |    66.66 |     100 |   84.21 | 93-94,109                     
-----------------------|---------|----------|---------|---------|-------------------------------
Test Suites: 1 failed, 1 total
Tests:       4 failed, 4 passed, 8 total
Snapshots:   0 total
Time:        1.4 s
Ran all test suites.

Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
@mikaelarguedas
Copy link
Contributor Author

mikaelarguedas commented May 11, 2024

I get test failures when running the tests locally, there's no obvious cause 🤔

Weird, I'll be honest I didnt run them as I thought it was covered by the pre-commits + CI.

Are these tests passing for you on master / v16 ? Are they supposed to pass locally ?
Some errors seem pretty github action specific -> [AssertionError: Expected RUNNER_TOOL_CACHE to be defined]

Edit:
Looks like test failure present on master/v16 so independant of this PR
Locally the Linux one fails because no env variable "GITHUB_WORKSPACE"
The windows one because the env var RUNNER_TOOL_CACHE is not defined
So it seems the test doesnt mock the github action environment successfully

Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

To be honest, I don't specifically remember running them locally, but it's been a while. I just assumed that tests were supposed to run fine locally/not in CI, but yeah you're right. Perhaps something changed when we bumped the @actions/* dependencies.

Tests pass as expected on CI, which now uses v20, so all good.

Thanks for the PR! I'll merge this, then wait for new CI for ros-tooling/action-ros-ci#870 and for it to be merged before creating new releases.

@christophebedard christophebedard merged commit 910b021 into ros-tooling:master May 11, 2024
20 checks passed
@mikaelarguedas mikaelarguedas deleted the node20 branch May 12, 2024 10:14
@christophebedard
Copy link
Member

I released this as 0.7.5/v0.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants