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

Add unit tests for DenseVector #310

Merged
merged 2 commits into from
Jul 3, 2022
Merged

Conversation

aadeg
Copy link
Contributor

@aadeg aadeg commented Jun 29, 2022

I've added some code to tests the DenseVector trait.

I used tarpaulin to evaluate the coverage improvement of the dense_vector.rs file: from 35% to 64%.

The tests are pretty simple. If you agree, I'd like to write some more unit tests to get confident with the library code and after that contributing on the algorithms.

I've a C++ dev background, and I've recently started learning Rust, so feel free to highlight any mistakes a did.

@mulimoen
Copy link
Collaborator

This looks great! You need to run cargo fmt to get the proper formatting, hence the failing CI

@aadeg aadeg force-pushed the tests_dense_vector branch from dc6d246 to 908cd4e Compare July 2, 2022 20:38
@aadeg
Copy link
Contributor Author

aadeg commented Jul 2, 2022

Sorry for that. Now it should be fixed.

@mulimoen
Copy link
Collaborator

mulimoen commented Jul 3, 2022

Seems miri is a bit more aggressive with borrowing rules. I've filed an error for this in rust-ndarray/ndarray#1178 and will ignore this test for now

@mulimoen mulimoen force-pushed the tests_dense_vector branch from 3ae726d to 62c056d Compare July 3, 2022 09:17
@mulimoen mulimoen merged commit 9ece75c into sparsemat:master Jul 3, 2022
@mulimoen
Copy link
Collaborator

mulimoen commented Jul 3, 2022

Thanks for your work on this @aadeg!

@aadeg aadeg deleted the tests_dense_vector branch July 3, 2022 20:54
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