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 cmake and ctest to build and run C++/python tests #25

Open
wants to merge 57 commits into
base: master
Choose a base branch
from

Conversation

SteveBronder
Copy link
Contributor

@SteveBronder SteveBronder commented Jun 30, 2023

Description

This adds cmake for running tests and is based on the Eigen update PR so needs to wait for that to merge before merging further. I also added boost since we need it for the airy test. I included a small example test with burst_test.cpp. It doesn't actually test anything but checks that google test (gtest) and cmake are running correctly.

The instructions in the readme have been updated so users can compile and run the C++ and python tests with the following.

cmake -S . -B "build" -DCMAKE_BUILD_TYPE=Debug
cd ./build
make -j4 test

This also adds builds that turn on the different sanitizers such as address (ASAN), leaks (LSAN), memory allocs (MSAN), and threads (TSAN). They can be turned on for example with

# Use address sanitizer
cmake -S . -B "build" -DCMAKE_BUILD_TYPE=ASAN

The sanitizers do pretty well, but have a hard time sanitizing std::list, so I switched all the lists to vectors. Since we only resize once at the beginning of solve and are then doing insertions into that preallocated memory this should also be faster.

Checklist:

  • I have performed a self-review of my own code
  • New and existing unit tests pass locally with my changes (pytest)
  • I have added tests that prove my fix is effective or that my feature works
  • Update version number: 1.3.0

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #25 (fd04807) into master (396c3e9) will not change coverage.
The diff coverage is n/a.

❗ Current head fd04807 differs from pull request most recent head 187c682. Consider uploading reports for the commit 187c682 to get more accurate results

@@         Coverage Diff          @@
##           master   #25   +/-   ##
====================================
  Coverage    0.00%     0           
====================================
  Files           1     0    -1     
  Lines          18     0   -18     
====================================
+ Misses         18     0   -18     

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@SteveBronder
Copy link
Contributor Author

FYI I need to debug the workflow script so there are going to be a few ... commits we can squash at the end

@SteveBronder
Copy link
Contributor Author

Alrighty! Ready for review!

@fruzsinaagocs
Copy link
Owner

Looks all good! Can you please bump the version no.?

@SteveBronder
Copy link
Contributor Author

@fruzsinaagocs I went through and updated the burst test, can you take a look that it looks right to you?

@SteveBronder
Copy link
Contributor Author

Also let's def squash this before merge. I had to use the github actions to debug the windows cmake

@SteveBronder
Copy link
Contributor Author

Fyi doing the burst test on windows we are finding a bug where the solver iterates one past the end of the list while doing the dense output. I'll look more into this tmrw. I can probably catch on linux with clangs mem sanitizer

@SteveBronder
Copy link
Contributor Author

Alrighty @fruzsinaagocs a few particulars when your looking this over

  • Check the readme and check the code for instillation runs on your machine. Everything is working fine here and on github actions (after much trial and error) but would like to make sure it works on another local machine.
  • Look over the burst_test to see if the answers EXPECT_NEAR() bounds checks are what you would expect. EXPECT_NEAR is used on the difference between the result of dx_burst(t) and the approximated answer. Is that right? I set them all to be low as they can be without failing.

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