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

test: add tests to @stdlib/ndarray/base/nullary #2350

Merged
merged 57 commits into from
Jul 14, 2024

Conversation

headlessNode
Copy link
Contributor

@headlessNode headlessNode commented Jun 10, 2024

Toward #2229

Description

What is the purpose of this pull request?

This pull request:

  • adds tests for @stdlib/ndarray/base/nullary

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

I added some edge cases for blockednullary2d up to blockednullary4d (same for related complex data types) to get 100% coverage. But I don't know if I used the most feasible methods. I'd like some insight into that, thanks.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@kgryte kgryte added the Tests Pull requests specifically adding tests. label Jun 10, 2024
@headlessNode
Copy link
Contributor Author

@Planeshifter @kgryte Please review, thanks.

@kgryte kgryte added the Needs Review A pull request which needs code review. label Jun 22, 2024
@headlessNode
Copy link
Contributor Author

@kgryte @Planeshifter Ping, reminder.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@headlessNode Thanks for working on this. I made some changes. The main thing is splitting out the tests for the various dimensions into separate files for future maintenance. We do this for other packages, especially those which require extensive test suites, and so it makes sense to follow suit here.

I've only refactored up to 2D. For 3D and up, the files should be similar to test.2d.js. One important change is that the unit tests test against the public interface, rather than the individual implementation files. By testing against the interface, we ensure that end-to-end tests work as expected and intended code paths are exercised.

To generate coverage reports, from the root project directory,

$ make test-cov TESTS_FILTER=".*/ndarray/base/nullary/.*"
$ make view-cov

To ensure relevant code paths were exercised, I also included additional tests for 2D, such as contiguity, non-contiguity, singleton dimensions, etc. I'd expect something similar to 3D+.

If you wouldn't mind continuing the test refactoring, then we can move full steam ahead and get these tests in! Thanks for your patience!

@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Jul 8, 2024
@headlessNode
Copy link
Contributor Author

@kgryte Sure. Thanks for the guidance.

@headlessNode
Copy link
Contributor Author

headlessNode commented Jul 10, 2024

@kgryte Hi, as I'm working through the refactoring, the expected array of dimension 7D+ is getting really large, 100s of elements. Here's the example shape, and stride for 7D

sh = [ 2, 2, 2, 2, 2, 2, 2 ];
st = [ 128, 64, 32, 16, 8, 2, 1 ];

Is this Okay or should I try to reduce it somehow?

@kgryte
Copy link
Member

kgryte commented Jul 10, 2024

You can use leading singleton dimensions. E.g., shape = [ 1, 1, 1, 1, 2, 2, 2 ] . To get full code coverage, you may need to use permutations (e.g., shape = [ 2, 1, 2, 1, 1, 1, 2 ], etc).

@headlessNode
Copy link
Contributor Author

Alright, thanks.

@headlessNode
Copy link
Contributor Author

@kgryte Please review, thanks.

@kgryte kgryte added Needs Review A pull request which needs code review. and removed Needs Changes Pull request which needs changes before being merged. labels Jul 11, 2024
@kgryte kgryte removed the Needs Review A pull request which needs code review. label Jul 14, 2024
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM. The primary missing tests needed at this point to achieve full test coverage are for blocked iteration. I've added tests for 2D blocked iteration. In order to avoid bogging this PR down further, I suggest we go ahead and merge. Any further tests can be added in follow-up PRs.

Thanks, @headlessNode, for working on this.

@kgryte kgryte merged commit 903c51c into stdlib-js:develop Jul 14, 2024
6 checks passed
@headlessNode headlessNode deleted the ndarray-nullary-tests branch July 14, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tests Pull requests specifically adding tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants