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

Add filterNodeArgumentsForWorkerThreads option #3336

Merged
merged 14 commits into from
Aug 20, 2024

Conversation

bojavou
Copy link
Contributor

@bojavou bojavou commented Jul 22, 2024

Adds a configuration option threadArgumentsFilter for filtering nodeArguments sent to worker threads. The option is supported only in config files due to accepting a function. When threads are disabled, the filter is ignored and the full unfiltered list of arguments is sent to worker processes.

main is currently failing tests. This patch adds no new failures. All new tests pass. The new feature is documented in 06-configuration.md.

This enabled me to use natives syntax in threads.

Closes #3207.

@bojavou
Copy link
Contributor Author

bojavou commented Jul 23, 2024

This check is failing:

Install and test AVA / Install dependencies without using a lockfile (pull_request)

I'm not clear why, and the new tests passed in the log. Is it possible this failure is unrelated?

Co-authored-by: Tommy <tmitchell7@uh.edu>
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Nice! Don't worry about unrelated test breakage, I can resolve those.

lib/cli.js Outdated Show resolved Hide resolved
docs/06-configuration.md Outdated Show resolved Hide resolved
bojavou and others added 2 commits August 2, 2024 13:46
Co-authored-by: Mark Wubben <mark@novemberborn.net>
@novemberborn
Copy link
Member

The Linux tests compare reporter output against a known-good version. That specific suite only runs on Linux because it's finicky enough as it is, without adding other environments into the mix.

However, it's failing through what looks like a change in this PR:

  �[31m✘ Internal error�[39m
  TypeError: execArgv is not iterable

  �[90mTypeError: execArgv is not iterable�[39m
  �[90m    at createWorker (/lib/fork.js:49:18)�[39m
  �[90m    at loadFork (/lib/fork.js:79:39)�[39m
  �[90m    at pMap.concurrency.concurrency (/lib/api.js:285:20)�[39m
  �[90m    at /node_modules/p-map/index.js:109:26�[39m

@bojavou
Copy link
Contributor Author

bojavou commented Aug 18, 2024

Not really sure what to change here but I'm moving toward an internal test runner anyway so I'm just dropping it.

@bojavou bojavou closed this Aug 18, 2024
@novemberborn
Copy link
Member

That's fine, I can take over.

@novemberborn novemberborn self-assigned this Aug 19, 2024
@novemberborn novemberborn reopened this Aug 19, 2024
lib/fork.js Outdated
@@ -60,7 +60,7 @@ const createWorker = (options, execArgv) => {
};
};

export default function loadFork(file, options, execArgv = process.execArgv) {
export default function loadFork(file, options, execArgv) {
Copy link
Member

Choose a reason for hiding this comment

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

This default assignment is still required.

@novemberborn novemberborn changed the title Add threadArgumentsFilter option Add filterNodeArgumentsForWorkerThreads option Aug 20, 2024
@novemberborn novemberborn merged commit 86185b4 into avajs:main Aug 20, 2024
16 checks passed
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.

Add option to filter invalid arguments from execArgv when launching worker threads
3 participants