-
Notifications
You must be signed in to change notification settings - Fork 171
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
fix: fixed tests for node #276
base: master
Are you sure you want to change the base?
Conversation
3386906
to
a443bbe
Compare
Would recommend something like import { join, dirname } from 'node:path'
import { fileURLToPath } from 'node:url'
fileURLToPath(dirname(import.meta.url))
You can copy relatively much from e.g. here: https://github.com/nuejs/nue/blob/master/packages/nuekit/test/nuekit.test.js Maybe something like the following incomplete example: import { join, dirname } from 'node:path'
import { fileURLToPath } from 'node:url'
import { promises as fs } from 'node:fs'
import { createKit } from '../src/nuekit.js'
const __dirname = fileURLToPath(dirname(import.meta.url))
// temporary directory
const root = join(__dirname, 'page-router-test')
const dist = join(root, '.dist')
const distDev = join(dist, 'dev')
// setup and teardown
beforeAll(async () => {
await fs.rm(dist, { recursive: true, force: true })
const nue = await createKit({ root }) // maybe not working (path problems...), but needed if not tested from root of monorepo but for this module only(?)
await nue.build()
})
// ...
function read(filePath) {
return fs.readFile(join(distDev, filePath), 'utf-8')
} There might still be |
a443bbe
to
ae7c7d8
Compare
Maybe avoid EDIT: (absolute) paths might be a problem with jest/... not sure, why though |
ae7c7d8
to
033ae4e
Compare
033ae4e
to
db28e0c
Compare
@nobkd Thank you for the review and hints! Currently the tests are failing because export async function getBuilder(is_esbuild) {
const actual_cwd = process.env.ACTUAL_CWD || process.cwd()
try {
return is_esbuild ? await import(resolve('esbuild', `file://${actual_cwd}/`)) : Bun
} catch {
throw 'Bundler not found. Please use Bun or install esbuild'
}
} I will take a look on that tomorrow. |
@mszmida looks like some checks were not succesful, so I'm hesitant to merge this |
Yes it's not running properly currently, because something (jest or...) handles path resolution differently, I think... Hidden as off-topic@tipiirai The tests currently don't run automatically when new contributors make a pull request. I don't know if there is a way to let them run automatically regardless, or not. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Hi, I'd recommend to keep the compatibility with older node.js as to give the best user ( developer)-experience as we can. Also a breaking-change which could be avoided is not good for any library. Maybe a polyfill for __dirname after a type check? |
This change fixes compatibility of tests with Node and Jest. Current Node LTS version is
v20.13.0
. On GitHub Actions we are using: