Skip to content

feat: port test_dataview to CTS#39

Open
bavulapati wants to merge 6 commits intonodejs:mainfrom
bavulapati:feat/port-test-dataview
Open

feat: port test_dataview to CTS#39
bavulapati wants to merge 6 commits intonodejs:mainfrom
bavulapati:feat/port-test-dataview

Conversation

@bavulapati
Copy link
Copy Markdown
Contributor

Ports
https://github.com/nodejs/node/tree/main/test/js-native-api/test_dataview from Node.js test suite to the CTS.
This test contains an experimental feature SharedArrayBuffer, resulting in usage of add_node_api_cts_experimental_addon().

@kraenhansen
Copy link
Copy Markdown
Contributor

Reviewed: C file is a verbatim copy of upstream, add_node_api_cts_experimental_addon is correct (matching the upstream NAPI_EXPERIMENTAL define), loadAddon is correctly guarded behind experimentalFeatures.sharedArrayBuffer, all upstream test cases preserved. The CMake refactor (dropping cmake_parse_arguments/SOURCES in favour of ${ARGN}) is safe — this PR introduces the first call site. "use strict" tracked in #41.

@bavulapati
Copy link
Copy Markdown
Contributor Author

@legendecas Fixed the CI issue. Can we run the tests again?

// Testing api calls for arrays
const test_dataview = loadAddon("test_dataview");

// Test for creating dataview with ArrayBuffer
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this test scoped in experimentalFeatures.sharedArrayBuffer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

node_api_is_sharedarraybuffer is experimental and this test uses it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CreateDataViewFromJSDataView does not use node_api_is_sharedarraybuffer though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

CreateDataView uses it.
Shall we split the test case similar to #44 ?
skipping after running CreateDataViewFromJSDataView assertions won't work as the loading of symbols will fail.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CreateDataView could also not using node_api_is_sharedarraybuffer. Alternatively, we can remove the type check in CreateDataView and rely on node-api implementors to check the types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. We can do that and that simplifies the migration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The docs say that napi_create_dataview supports both array buffer and shared array buffer but fails with invalid argument on node v20 & v22

Ports
[https://github.com/nodejs/node/tree/main/test/js-native-api/test_dataview](test_dataview)
from Node.js test suite to the CTS.
This test contains an experimental feature `SharedArrayBuffer`,
resulting in usage of `add_node_api_cts_experimental_addon()`.

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
node_api_is_sharedarraybuffer is added in v24.9.0
https://nodejs.org/api/n-api.html#node-api-is-sharedarraybuffer

Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
@bavulapati bavulapati force-pushed the feat/port-test-dataview branch from f82ec4f to 3ca85da Compare April 9, 2026 04:53
@bavulapati bavulapati requested a review from legendecas April 9, 2026 04:55
This reverts commit 3ca85da.
Signed-off-by: Balakrishna Avulapati <ba@bavulapati.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Need Triage

Development

Successfully merging this pull request may close these issues.

3 participants