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.

Sign up for GitHub

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

test: remove outdated V8 flag #895

Closed
wants to merge 2 commits into from
Closed

test: remove outdated V8 flag #895

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29815b3
test: remove outdated V8 flag
RaisinTen Jan 31, 2021
1d8a73d
fixup! test: remove outdated V8 flag
RaisinTen Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ const majorNodeVersion = process.versions.node.split('.')[0];

if (typeof global.gc !== 'function') {
// Construct the correct (version-dependent) command-line args.
let args = ['--expose-gc', '--no-concurrent-array-buffer-freeing'];
let args = ['--expose-gc'];
Copy link
Member

Choose a reason for hiding this comment

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

Did you check the history of why this was added in the first place. I'm wondering if adding it needs to be Node.js version dependent versus just removing completely (ie might results in failures on older Node.js versions...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't check it before making this PR. I checked now and the PR that introduced this is: #737. It fixed this issue: #735. I'm not sure whether removing this flag would end up causing the failure again because we don't have any Windows tests in the CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a check to conditionally add the option if v8 has a major version that is less than 9. PTAL.

const majorV8Version = process.versions.v8.split('.')[0];
if (majorV8Version < 9) {
args.push('--no-concurrent-array-buffer-freeing');
}
if (majorNodeVersion >= 14) {
args.push('--no-concurrent-array-buffer-sweeping');
}
Expand Down