Skip to content

see if this helps with test reliability#192

Merged
heskew merged 1 commit intomainfrom
testupd
Mar 5, 2026
Merged

see if this helps with test reliability#192
heskew merged 1 commit intomainfrom
testupd

Conversation

@heskew
Copy link
Member

@heskew heskew commented Mar 4, 2026

No description provided.

@heskew heskew marked this pull request as ready for review March 5, 2026 06:05
@heskew heskew requested a review from a team as a code owner March 5, 2026 06:05
Copy link
Member

@kriszyp kriszyp left a comment

Choose a reason for hiding this comment

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

LGTM

import { setupTestApp } from './setupTestApp.mjs';
import { get as env_get, setProperty } from '#js/utility/environment/environmentManager';
import { createRequire } from 'node:module';
const { get: env_get, setProperty } = createRequire(import.meta.url)('#js/utility/environment/environmentManager');
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here? Is this fixing a lint warning or an actual functionality bug?

I've seen a lot of warnings from trying to import CommonJS files. I usually can resolve them by doing:

import * as environmentManager from '#js/utility/environment/environmentManager';
const { get: env_get, setProperty } = environmentManager ;

I don't love this pattern, but then again I don't love creating a require() instance either.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's addressing this error that was only seen in node 24

 Exception during run: file:///home/runner/work/harper/harper/unitTests/apiTests/setupTestApp.mjs:3
import { setProperty } from '#js/utility/environment/environmentManager';
         ^^^^^^^^^^^
SyntaxError: The requested module '#js/utility/environment/environmentManager' does not provide an export named 'setProperty'
    at #asyncInstantiate (node:internal/modules/esm/module_job:319:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:422:5)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:661:26)
    at async formattedImport (/home/runner/work/harper/harper/node_modules/mocha/lib/nodejs/esm-utils.js:10:14)
    at async exports.loadFilesAsync (/home/runner/work/harper/harper/node_modules/mocha/lib/nodejs/esm-utils.js:152:20)
    at async singleRun (/home/runner/work/harper/harper/node_modules/mocha/lib/cli/run-helpers.js:168:3)
    at async exports.handler (/home/runner/work/harper/harper/node_modules/mocha/lib/cli/run.js:379:5)

Copy link
Member Author

Choose a reason for hiding this comment

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

All for any other better feeling solution that addresses the issue. This is just the first one that seemed to do the trick...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is the same issue I was running into yesterday with Harper v5 and Node.js 22.17.0. I think the import * as whatever from 'thing', then destructuring into a variable is the cleanest solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give that a shot here before merging. don't want to get an ugly pattern started... brb

Copy link
Member Author

Choose a reason for hiding this comment

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

updated and rebased, so now this pr is only this change

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

Ha, it works!

@heskew heskew merged commit 53e0bde into main Mar 5, 2026
2 checks passed
@heskew heskew deleted the testupd branch March 5, 2026 17:16
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.

4 participants