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

Jump to bottom

Write tests for Array class #363

Closed
wants to merge 2 commits into from
Closed

Write tests for Array class #363

wants to merge 2 commits into from

Conversation

yjaeseok
Copy link
Contributor

@yjaeseok yjaeseok commented Oct 2, 2018

I would like to write tests fro array class.

ISSUE=#332

@yjaeseok
Write tests for Array class
cc6a9b9 
I would like to write tests fro array class.

ISSUE=nodejs#332
mhdawson
mhdawson approved these changes Oct 2, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM. Might be useful to add a few tests that validate that arrays created in JavaScript can be used in natives and vice versa, but happy to have those go in through a separate PR.

gabrielschulhof
gabrielschulhof approved these changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

Just a few nits.

test/basic_types/array.js Outdated Show resolved Hide resolved
test/basic_types/array.js Outdated Show resolved Hide resolved
@yjaeseok
Copy link
Contributor Author

yjaeseok commented Oct 9, 2018

@mhdawson okay I will request PR after this patch.

@yjaeseok
Copy link
Contributor Author

yjaeseok commented Oct 9, 2018

@mhdawson @gabrielschulhof please review it again. :)

mhdawson
mhdawson approved these changes Oct 9, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM and thanks for the help.

mhdawson pushed a commit that referenced this pull request Oct 22, 2018
@yjaeseok @mhdawson
test: write tests for Array class
67b7db0 
add tests for array class.

PR-URL: #363
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@mhdawson
Copy link
Member

Landed as 67b7db0

@mhdawson mhdawson closed this Oct 22, 2018
@JckXia JckXia mentioned this pull request Apr 8, 2021
Closed
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
test: write tests for Array class
0ef89e2 
add tests for array class.

PR-URL: nodejs/node-addon-api#363
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
test: write tests for Array class
89e7a0d 
add tests for array class.

PR-URL: nodejs/node-addon-api#363
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
test: write tests for Array class
e55b390 
add tests for array class.

PR-URL: nodejs/node-addon-api#363
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
test: write tests for Array class
fa5e345 
add tests for array class.

PR-URL: nodejs/node-addon-api#363
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjakeyoon jjakeyoon jjakeyoon left review comments

@mhdawson mhdawson mhdawson approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yjaeseok @mhdawson @thefourtheye @gabrielschulhof @jjakeyoon