refactor: migrate tests from tape to node:test#178
Conversation
e3d0ce9 to
6c4f018
Compare
1ab389a to
6951d6a
Compare
metcoder95
left a comment
There was a problem hiding this comment.
rest lgtm, just question on removing the browser tests
There was a problem hiding this comment.
Shall we remove it?
I removed the section related to test-browser from ci.yml, so I think it should be okay now we should then see if it is aligned with the others fastify-libraries
climba03003
left a comment
There was a problem hiding this comment.
Browser test exists for reason, it allows us to check whether the code break in browser or not.
For example, Buffer check inside the codebase is not compatible in browser.
Okay, thank you for the clarification! so we have to leave them as they are: does that mean we can't replace |
|
I thinking about it, we can still replace Anyway, I made a commit and put the browser tests back into CI , see commit here: fe282a8 |
| "test:unit": "node --test test/*.test.js", | ||
| "test:typescript": "tstyche", | ||
| "test:browser": "airtap test/*.test.js" | ||
| "test:browser": "node --test test/*.test.js" |
There was a problem hiding this comment.
It doesn't seems to use playwright for testing?
Only installing playwright do not means it is inside the browser environment.
See https://playwright.dev/docs/ci#github-actions
There was a problem hiding this comment.
Okay, I think I've fixed it. I added a file named browser.spec.js ( see commit here: 846caba) to run the test in the browser. Using Playwright in automation script mode (via browser.spec.js) allows us to test the real environment in a more direct, clean way, without any “magic” behind the scenes like airtap ( before that, Airtap read everything from the index.js file) , thereby filling the gap in cross-browser testing. Without this script, we would no longer have visibility into the library’s behavior within the JavaScript engines of various browsers (Chromium, Firefox, WebKit).
The new test does not merely re-run the unit tests (which remain in Node), but specifically focuses on the two critical issues of the client-side environment: the absence of Node-only objects (such as Buffer) and the validation of the prototype pollution protection logic within the browser's JavaScript engine.
9dfe3da to
846caba
Compare
|
|
||
| const { test } = require('tape') | ||
| const { test } = require('node:test') | ||
| const assert = require('node:assert') |
There was a problem hiding this comment.
Away from a computer atm and reviewing from phone is hard, but any reason to use this over the testcontext assert inside of test functions?
There was a problem hiding this comment.
Away from a computer atm and reviewing from phone is hard, but any reason to use this over the testcontext assert inside of
testfunctions?
Yes, you can do that with TestContext as well ( see commit here: cb48c17) ; you can do it both ways 😉
Proposal:
This PR migrates the test suite from
tapeto the nativenode:testrunner and node:assert module. Since tape is no longer actively maintained and Node.js now provides a robust built-in testing solution, this migration reduces external dependencies and align the project with other repositories in the fastify ecosystemChanges made:
tapetonode:testairtap,airtap-playwrightwithnode:testalong withplaywright