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

build: run tests on node.js v21 nightly #199

Merged
merged 24 commits into from
May 16, 2023
Merged

Conversation

aladdin-add
Copy link
Collaborator

No description provided.

@iambumblehead
Copy link
Owner

iambumblehead commented Apr 19, 2023

Hey everyone I tried resolving things here but encountered the upstream error reported here nodejs/node#47614

Also, there seems to be a separate issue where the loader isn't engaged by the ava unit tests. I tried using this alternative syntax in the tests-ava package.json but the loader wasn't engaged...

{
  "ava": {
    "nodeArguments": [
      "--loader=esmock"
    ]
  }
}

Feel free to leave any input or advice. Thanks for pushing v20 tests @aladdin-add

@iambumblehead
Copy link
Owner

the ava issue is reproduced and reported here avajs/ava#3195

@aladdin-add
Copy link
Collaborator Author

as the --loader is still experimental, I like to add a task to test it on node.js nightly build, and file an issue if it fails. WDYT?

will give it a try if no objections. 😄

@aladdin-add
Copy link
Collaborator Author

looks like the node.js v20-nightly is still failing.

@iambumblehead
Copy link
Owner

this branch is rebased on the latest master which handles async import.meta.resolve differently and specifies node-versions <=19.9.0

@iambumblehead
Copy link
Owner

The nightly used in the tests appears to be an older nightly 20.0.0-nightly202304181c3d741cb0 as indicated by the timestamp in the name. We will want 20230514 seen here https://nodejs.org/download/nightly/

@iambumblehead
Copy link
Owner

iambumblehead commented May 16, 2023

tests for all folders fail when run from the latest nightly... I won't have bandwidth to investigate further until end of week

the problem appears to be here https://github.com/iambumblehead/esmock/blob/master/src/esmockLoader.js#L37

global.esmockTreeIdGet(parentURL.match(esmkIdRe)[0].split('=')[1])

The loader and test contexts are separated. we don't really need to access the test namespace this way and we could instead import the mocked module with the full serialized uri. The reason we exchanged the small esmkId with the full url is, when the test failed the longer url would be printed by the failing test adding noise to the failing test which made the stacktrace harder to read...

@iambumblehead
Copy link
Owner

iambumblehead commented May 16, 2023

if anyone here knows generally the way data should be communicated from test to loader, feel free to leave incomplete advice. If there is a generally-recommended approach, I might be able to add it this evening

@iambumblehead
Copy link
Owner

let's try to implement this way, if there are no other suggestions nodejs/node#44710 (comment)

@iambumblehead
Copy link
Owner

I have a passing test here that uses worker thread to define the longer uri in the loader, but cannot use this machine to push up any changes so it might take some time before anything reaches this branch

@iambumblehead
Copy link
Owner

I thought these changes were developed around node 21 but it looks like node 19 was used :/

@iambumblehead
Copy link
Owner

When using node 21 nightly here, changes fail for ava's test folder, but they pass for the other test-runners.

src/esmockCache.js Outdated Show resolved Hide resolved
@iambumblehead
Copy link
Owner

ava does not work with node v20+ loaders at all https://github.com/iambumblehead/nodejs-v20-loader-ava-noop

@iambumblehead
Copy link
Owner

iambumblehead commented May 16, 2023

@koshic @aladdin-add @Swivelgames @tripodsan the tests are passing yay. feel free to leave opinions, concerns or suggestions

@iambumblehead iambumblehead requested a review from koshic May 16, 2023 09:58
@flovogt
Copy link

flovogt commented May 16, 2023

title says enable node v20 but currently only nightly-v21 is used. That should be reflected in the title or in the code

@iambumblehead
Copy link
Owner

@flovogt okay, which do you recommend?

@iambumblehead iambumblehead changed the title build: run tests on node.js v20 build: run tests on node.js v21 nightly May 16, 2023
@iambumblehead
Copy link
Owner

@flovogt the title is updated

@koshic
Copy link
Collaborator

koshic commented May 16, 2023

I think it's better to merge it ASAP and publish new package version. We can improve loader-userspace logic interaction later.

@iambumblehead iambumblehead merged commit 0a89895 into master May 16, 2023
@iambumblehead iambumblehead deleted the aladdin-add-patch-1 branch May 16, 2023 11:56
@iambumblehead
Copy link
Owner

a new PR adding node v20 tests to the ci pipeline is created #202

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.

5 participants