chore(netlify): use Node.js for testing #158

abidjappie · 2024-02-07T00:19:26Z

Changes

This PR migrates the @astro/netlify tests to Node.js (original issue: ☂️ Move Adapters tests to node:test #144)
Will include changeset once it's completed pnpm exec changeset

Challenges

Node.js does not support snapshot matching for now (Snapshot testing nodejs/node#48260)
Currently investigating failing test related to fs

Testing

For now, run the following command in packages/netlify

node --test **/*.test.js

Docs

For now there is no documentation, but that might change

changeset-bot · 2024-02-07T00:19:29Z

⚠️ No Changeset found

Latest commit: 260fa6f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

alexanderniebuhr · 2024-02-08T06:40:06Z

@abidjappie Thanks for the PR. Maybe you can consult with @Skn0tt to see if we can rewrite the snapshot test to be still valuable without a snapshot :)

…etup and update package.

…dentify failures and fixes some issues of leaking test state.

abidjappie · 2024-02-08T14:41:16Z

Something I noticed is that either Image CDN or Middleware tests will fail when running on local (when one passes the other fails). I suspect it's related to how it builds the fixtures... in either case, it cannot find the file.

Image CDN:

[Error: ENOENT: no such file or directory, open '/Users/abid.jappie/code/withastro-adapters/packages/netlify/test/functions/fixtures/middleware/dist/_astro/astronaut.jcSXTCiO_1Hy9L1.webp'] { errno: -2, code: 'ENOENT', syscall: 'open', path: '/Users/abid.jappie/code/withastro-adapters/packages/netlify/test/functions/fixtures/middleware/dist/_astro/astronaut.jcSXTCiO_1Hy9L1.webp' }

Middleware:

 [Error: ENOENT: no such file or directory, open '/Users/abid.jappie/code/withastro-adapters/packages/netlify/test/functions/fixtures/middleware/dist/_astro/astronaut.jcSXTCiO_1Hy9L1.webp'] { errno: -2, code: 'ENOENT', syscall: 'open', path: '/Users/abid.jappie/code/withastro-adapters/packages/netlify/test/functions/fixtures/middleware/dist/_astro/astronaut.jcSXTCiO_1Hy9L1.webp' }

packages/netlify/test/functions/redirects.test.js

Skn0tt

looks good on first sight!

Skn0tt · 2024-02-09T11:09:14Z

it cannot find the file.

Hmm, I wonder wether that also happens on CI 🤔 Mind sharing the full stacktraces, to see where the fs.open call originates?

alexanderniebuhr · 2024-02-09T13:28:03Z

I'll also migrate this script https://github.com/withastro/astro/blob/main/scripts/cmd/test.js over to the adapters repo, because using the Node's cli has some downsides. Once I've migrated it, we can use it for this PR too.

alexanderniebuhr · 2024-02-11T07:39:16Z

@abidjappie, @Skn0tt What is the state? Do we need still to figure stuff out, or can we update this PR with latest main and get this merged?

I also just merged #150 which adds support for astro-script test

abidjappie · 2024-02-13T06:55:55Z

@alexanderniebuhr I'll look into the feedback sometime today, and try to get some stack traces.
After that I would like to run the CI tests so we can see if it errors out or not before proceeding 🙇

abidjappie · 2024-02-13T12:53:27Z

@Skn0tt

Hmm, I wonder wether that also happens on CI 🤔 Mind sharing the full stacktraces, to see where the fs.open call originates?

I'm running the tests via node --test. In my attempt to find more information I surrounded both the test case, and the before each with a catch... and it seems to be occurring in the beforeEach level. Specifically:

await fixture.build();

My guess is that the test are running concurrently, which is why it fails in different places each time. I wrapped it in console.trace:

Trace: Error: ENOENT: no such file or directory, stat '/Users/abid.jappie/code/withastro-adapters/packages/netlify/test/functions/fixtures/middleware/.netlify/functions-internal/ssr/manifest_n9sb-Uc7.mjs'
    at Object.statSync (node:fs:1690:3)
    at removeEmptyDirs (file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/fs/index.js:12:11)
    at file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/fs/index.js:18:7
    at Array.map (<anonymous>)
    at removeEmptyDirs (file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/fs/index.js:16:11)
    at cleanStaticOutput (file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/build/static-build.js:314:5)
    at async staticBuild (file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/build/static-build.js:104:7)
    at async AstroBuilder.build (file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/build/index.js:138:5)
    at async AstroBuilder.run (file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/build/index.js:169:7)
    at async build (file:///Users/abid.jappie/code/withastro-adapters/node_modules/.pnpm/astro@4.3.5_@types+node@18.19.2_typescript@5.3.2/node_modules/astro/dist/core/build/index.js:47:3) {
  errno: -2,
  syscall: 'stat',
  code: 'ENOENT',
  path: '/Users/abid.jappie/code/withastro-adapters/packages/netlify/test/functions/fixtures/middleware/.netlify/functions-internal/ssr/manifest_n9sb-Uc7.mjs'
}
    at TestContext.<anonymous> (file:///Users/abid.jappie/code/withastro-adapters/packages/netlify/test/functions/edge-middleware.test.js:18:13)
    at async TestHook.run (node:internal/test_runner/test:574:9)
    at async node:internal/test_runner/test:509:9
    at async Suite.runHook (node:internal/test_runner/test:507:7)
    at async Test.run (node:internal/test_runner/test:549:9)
    at async Suite.processPendingSubtests (node:internal/test_runner/test:318:7)

Skn0tt · 2024-02-13T13:00:49Z

Could be because of concurrency, yes! I see you replaced before with beforeEach in the edge-middleware test, what happens when you switch that back to before?

abidjappie · 2024-02-13T13:00:50Z

@alexanderniebuhr can you help me to get the test running using astro-scripts.
I pulled latest branch and tried moving to it, but I get no output:

> @astrojs/netlify@5.1.1 test /Users/abid.jappie/code/withastro-adapters/packages/netlify
> pnpm run test-fn && pnpm run test-static

> @astrojs/netlify@5.1.1 test-fn /Users/abid.jappie/code/withastro-adapters/packages/netlify
> astro-scripts --test ./test/functions/*.test.js

> @astrojs/netlify@5.1.1 test-static /Users/abid.jappie/code/withastro-adapters/packages/netlify
> astro-scripts --test ./test/static/*.test.js

node --test **/*.test.js works fine.

abidjappie · 2024-02-13T14:13:39Z

@Skn0tt
I managed to get edge-middleware.test.js to pass, there are two parts:

You are totally right that changing before to beforeEach was part of the solution.
We also need to run the await fixture.clean() in the after

Unfortunately, I think this might solve the issue of file access overlapping in a single test, I think that different test files can still have this effect on one another: edge-middleware now consistently passes, but image-cdn consistently fails.

I can easily break edge-middleware by adding await fixture.clean() inside of image-cdn.test.js.
Is there someway to add a test specific config to test-utils.js or can we somehow isolate the tests/ ensure they don't run in parallel?

This also works for running image-cdn.test.js in isolation: await fixture.clean() after each it:

node --test **/image-cdn.test.js
▶ Image CDN
  ▶ when running outside of netlify
    ✔ does not enable Image CDN (714.118083ms)
  ▶ when running outside of netlify (714.634083ms)

  ▶ when running inside of netlify
    ✔ enables Netlify Image CDN (326.024042ms)
    ✔ respects image CDN opt-out (434.420417ms)
  ▶ when running inside of netlify (760.604291ms)

▶ Image CDN (1475.79075ms)

ℹ tests 3
ℹ suites 3
ℹ pass 3
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 2056.534583

Skn0tt · 2024-02-13T14:18:43Z

Hmm, I'm not sure I follow. Could you push those changes? then I can try running it locally to see what's going wrong.

alexanderniebuhr · 2024-02-13T14:43:12Z

@abidjappie let me check astro-scripts

packages/netlify/package.json

….test.js is still failing.

abidjappie · 2024-02-14T03:28:59Z

@Skn0tt

Hmm, I'm not sure I follow. Could you push those changes? then I can try running it locally to see what's going wrong.

You can try with this branch. State: edge-middleware should pass, image-cdn seems to fail.
f26386e

If you surround each case in image-cdn with test.only you can of course get it to pass.

alexanderniebuhr · 2024-02-14T07:31:54Z

This is interesting, using astro-scripts test does not run tests in parallel. See: https://github.com/withastro/adapters/blob/main/scripts/cmd/test.js#L42-L54

abidjappie · 2024-02-14T09:26:30Z

@alexanderniebuhr true. I also noticed that running with astro-scripts test ./test/functions/*.test.js only runs the first test in the folder.

pnpm run test-fn

> @astrojs/netlify@5.1.2 test-fn /Users/abid.jappie/code/withastro-adapters/packages/netlify
> astro-scripts test ./test/functions/*.test.js

▶ Cookies
  ✔ Can set multiple (13.354375ms)
  ✔ renders dynamic 404 page (4.59525ms)
▶ Cookies (597.759083ms)

ℹ tests 2
ℹ suites 1
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 1190.360458

I'm not sure if it supports recursive, but this works for me:

    "test-fn": "for f in test/functions/*.test.js; do echo $f; astro-scripts test $f; done"

Should we update the script ?

…ctivity.

abidjappie · 2024-02-14T10:02:46Z

@Skn0tt I've pushed some changes now. You can get all tests to pass.

for f in test/functions/*.test.js; do echo $f; pnpm astro-scripts test $f; done

It may be related to extraneous asynchronous activity since removing the env cleanup in afa37ea somehow fixed it.

I think it may need further investigation at some point, but I'm just relieved it's behaving somewhat consistently on my machine now.

Skn0tt

Looks wonderful! Good job at getting all the tests to pass under a new test runner :D

Left a tiny nit, but nothing blocking.

packages/netlify/test/functions/redirects.test.js

packages/netlify/package.json

alexanderniebuhr · 2024-02-14T18:58:19Z

@abidjappie Thank you so much for your contribution. Appreciate the help ❤️ I'll take over from here and make sure this get's merged asap.

github-actions bot added the pkg: netlify label Feb 7, 2024

abidjappie marked this pull request as draft February 7, 2024 00:24

abidjappie added 3 commits February 8, 2024 22:36

Update the test cases using RegEx. TODO: implement snapshot, update s…

e237800

…etup and update package.

Updated remaining cases with RegEx. TODO: remove unused imports.

acde465

Commented out snapshot for now.

7c6c22f

abidjappie force-pushed the chore_use_node_test_netlify branch from fa78fa0 to 7c6c22f Compare February 8, 2024 13:36

abidjappie added 2 commits February 8, 2024 23:21

Set up and clean up before each/ after each case makes it easier to i…

edbaa9c

…dentify failures and fixes some issues of leaking test state.

Clean up env in image-cdn.test.

554cdda

Skn0tt reviewed Feb 9, 2024

View reviewed changes

packages/netlify/test/functions/redirects.test.js Outdated Show resolved Hide resolved

Skn0tt reviewed Feb 9, 2024

View reviewed changes

alexanderniebuhr changed the title ~~chore(@astro/netlify): use Node.js for testing~~ chore(netlify): use Node.js for testing Feb 11, 2024

Fixed merge conflicts.

810fb86

Remove snapshot test in favour of inline. Remove usage of delete key.

16d965a

alexanderniebuhr reviewed Feb 13, 2024

View reviewed changes

packages/netlify/package.json Outdated Show resolved Hide resolved

alexanderniebuhr reviewed Feb 13, 2024

View reviewed changes

packages/netlify/package.json Outdated Show resolved Hide resolved

alexanderniebuhr reviewed Feb 13, 2024

View reviewed changes

packages/netlify/package.json Outdated Show resolved Hide resolved

Update packages/netlify/package.json

02bc5f3

alexanderniebuhr and others added 4 commits February 13, 2024 20:54

Update packages/netlify/package.json

a109531

Update packages/netlify/package.json

fb3d338

fix lockfile

3eb4b25

Fixed astro-scripts scripts. Fixed edge-middleware.test.js, image-cdn…

f26386e

….test.js is still failing.

Merge branch 'main' into chore_use_node_test_netlify

2d97ce8

All test pass somehow, it may be related to extraneous asynchronous a…

afa37ea

…ctivity.

Skn0tt approved these changes Feb 14, 2024

View reviewed changes

packages/netlify/test/functions/redirects.test.js Show resolved Hide resolved

alexanderniebuhr reviewed Feb 14, 2024

View reviewed changes

packages/netlify/package.json Outdated Show resolved Hide resolved

alexanderniebuhr reviewed Feb 14, 2024

View reviewed changes

packages/netlify/package.json Outdated Show resolved Hide resolved

alexanderniebuhr reviewed Feb 14, 2024

View reviewed changes

packages/netlify/package.json Outdated Show resolved Hide resolved

alexanderniebuhr added 4 commits February 14, 2024 15:33

Update packages/netlify/package.json

d3dd05a

Update packages/netlify/package.json

d6b2c5d

Update packages/netlify/package.json

6e16afc

remove snapshot file

260fa6f

alexanderniebuhr marked this pull request as ready for review February 14, 2024 18:58

alexanderniebuhr approved these changes Feb 14, 2024

View reviewed changes

ematipico approved these changes Feb 16, 2024

View reviewed changes

alexanderniebuhr merged commit f52bf88 into withastro:main Feb 16, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(netlify): use Node.js for testing #158

chore(netlify): use Node.js for testing #158

abidjappie commented Feb 7, 2024 •

edited

Loading

changeset-bot bot commented Feb 7, 2024 •

edited

Loading

alexanderniebuhr commented Feb 8, 2024

abidjappie commented Feb 8, 2024

Skn0tt left a comment

Skn0tt commented Feb 9, 2024

alexanderniebuhr commented Feb 9, 2024

alexanderniebuhr commented Feb 11, 2024

abidjappie commented Feb 13, 2024

abidjappie commented Feb 13, 2024

Skn0tt commented Feb 13, 2024

abidjappie commented Feb 13, 2024

abidjappie commented Feb 13, 2024 •

edited

Loading

Skn0tt commented Feb 13, 2024

alexanderniebuhr commented Feb 13, 2024 •

edited

Loading

abidjappie commented Feb 14, 2024

alexanderniebuhr commented Feb 14, 2024

abidjappie commented Feb 14, 2024 •

edited

Loading

abidjappie commented Feb 14, 2024 •

edited

Loading

Skn0tt left a comment

alexanderniebuhr commented Feb 14, 2024 •

edited

Loading

chore(netlify): use Node.js for testing #158

chore(netlify): use Node.js for testing #158

Conversation

abidjappie commented Feb 7, 2024 • edited Loading

Changes

Challenges

Testing

Docs

changeset-bot bot commented Feb 7, 2024 • edited Loading

⚠️ No Changeset found

alexanderniebuhr commented Feb 8, 2024

abidjappie commented Feb 8, 2024

Skn0tt left a comment

Choose a reason for hiding this comment

Skn0tt commented Feb 9, 2024

alexanderniebuhr commented Feb 9, 2024

alexanderniebuhr commented Feb 11, 2024

abidjappie commented Feb 13, 2024

abidjappie commented Feb 13, 2024

Skn0tt commented Feb 13, 2024

abidjappie commented Feb 13, 2024

abidjappie commented Feb 13, 2024 • edited Loading

Skn0tt commented Feb 13, 2024

alexanderniebuhr commented Feb 13, 2024 • edited Loading

abidjappie commented Feb 14, 2024

alexanderniebuhr commented Feb 14, 2024

abidjappie commented Feb 14, 2024 • edited Loading

abidjappie commented Feb 14, 2024 • edited Loading

Skn0tt left a comment

Choose a reason for hiding this comment

alexanderniebuhr commented Feb 14, 2024 • edited Loading

abidjappie commented Feb 7, 2024 •

edited

Loading

changeset-bot bot commented Feb 7, 2024 •

edited

Loading

abidjappie commented Feb 13, 2024 •

edited

Loading

alexanderniebuhr commented Feb 13, 2024 •

edited

Loading

abidjappie commented Feb 14, 2024 •

edited

Loading

abidjappie commented Feb 14, 2024 •

edited

Loading

alexanderniebuhr commented Feb 14, 2024 •

edited

Loading