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

Split the object test into smaller tests (get/set/delete/hasOwnProperty) #183

Closed
wants to merge 1 commit into from

Conversation

romandev
Copy link
Contributor

@romandev romandev commented Nov 6, 2017

This change breaks the object test into smaller tests for the following
reasons:

  • Smaller tests are better than very large one. (e.g. readability)
  • Before this patch, we have missed some test cases. For example,
    there are four overloading versions of the deleteProperty() method,
    but we haven't tested them all until now.

@romandev
Copy link
Contributor Author

romandev commented Nov 6, 2017

@mhdawson PTAL

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.

Overall LGTM. I'm on vacation for the next week so I'll look at landing when I get back or if another reviewer takes a look between now and then they can go head earlier.

@romandev
Copy link
Contributor Author

romandev commented Nov 9, 2017

@mhdawson Thank you for review and I'll request another reviewer. Have a good vacation :)

@romandev
Copy link
Contributor Author

romandev commented Nov 9, 2017

@addaleax Could you please review this patch if you have a minute?

@mhdawson
Copy link
Member

Ran the tests locally on linux and I got this failure:

Starting test suite

Running test 'arraybuffer'
Running test 'asyncworker'
Running test 'buffer'
Running test 'error'
Running test 'external'
Running test 'function'
Running test 'name'
Running test 'object/delete_property'
Running test 'object/get_property'
Running test 'object/has_own_property'
Running test 'object/object'
Running test 'object/set_property'
Tests aborted with SIGSEGV
npm ERR! Test failed.  See above for more details.

With this PR backed out the tests ran ok.

@romandev can you take a look. You can run the tests with npm-test

This change breaks the object test into smaller tests for the following
reasons:
  - Smaller tests are better than very large one. (e.g. readability)
  - Before this patch, we have missed some test cases. For example,
    there are four overloading versions of the deleteProperty() method,
    but we haven't tested them all until now.
@romandev
Copy link
Contributor Author

@mhdawson Done. I've just fixed the error. Thank you!

mhdawson pushed a commit that referenced this pull request Nov 17, 2017
Split the object test into smaller tests (get/set/delete/hasOwnProperty)

This change breaks the object test into smaller tests for the following
reasons:
  - Smaller tests are better than very large one. (e.g. readability)
  - Before this patch, we have missed some test cases. For example,
    there are four overloading versions of the deleteProperty() method,
    but we haven't tested them all until now.

PR-URL: #183
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

@romandev thanks, landed as e90a80b

@mhdawson mhdawson closed this Nov 17, 2017
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Split the object test into smaller tests (get/set/delete/hasOwnProperty)

This change breaks the object test into smaller tests for the following
reasons:
  - Smaller tests are better than very large one. (e.g. readability)
  - Before this patch, we have missed some test cases. For example,
    there are four overloading versions of the deleteProperty() method,
    but we haven't tested them all until now.

PR-URL: nodejs/node-addon-api#183
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
Split the object test into smaller tests (get/set/delete/hasOwnProperty)

This change breaks the object test into smaller tests for the following
reasons:
  - Smaller tests are better than very large one. (e.g. readability)
  - Before this patch, we have missed some test cases. For example,
    there are four overloading versions of the deleteProperty() method,
    but we haven't tested them all until now.

PR-URL: nodejs/node-addon-api#183
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
Split the object test into smaller tests (get/set/delete/hasOwnProperty)

This change breaks the object test into smaller tests for the following
reasons:
  - Smaller tests are better than very large one. (e.g. readability)
  - Before this patch, we have missed some test cases. For example,
    there are four overloading versions of the deleteProperty() method,
    but we haven't tested them all until now.

PR-URL: nodejs/node-addon-api#183
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Split the object test into smaller tests (get/set/delete/hasOwnProperty)

This change breaks the object test into smaller tests for the following
reasons:
  - Smaller tests are better than very large one. (e.g. readability)
  - Before this patch, we have missed some test cases. For example,
    there are four overloading versions of the deleteProperty() method,
    but we haven't tested them all until now.

PR-URL: nodejs/node-addon-api#183
Reviewed-By: Michael Dawson <[email protected]>
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.

2 participants