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 testing framework and tests for existing code (and assorted fixes and breaking API changes) #23

Merged
merged 41 commits into from
Oct 15, 2023

Conversation

Sophon96
Copy link
Member

@Sophon96 Sophon96 commented Oct 2, 2023

Using catch2 and integration with meson's testing harness (so tests can be run with meson test).

@Sophon96
Copy link
Member Author

Sophon96 commented Oct 2, 2023

Incomplete, blocked for test requirements from @adamhutchings for the matrix and vector classes.

@adamhutchings
Copy link
Contributor

Are we merging this after #25?

@Sophon96
Copy link
Member Author

Sophon96 commented Oct 3, 2023

Are we merging this after #25?

No, I need to write tests for the existing code as well (though this is blocked by #25 as well).

tests/math/matrix.cpp Outdated Show resolved Hide resolved
@adamhutchings
Copy link
Contributor

Is development on this able to proceed or are we still waiting on something else?

@Sophon96
Copy link
Member Author

Sophon96 commented Oct 5, 2023

Is development on this able to proceed or are we still waiting on something else?

Waiting on nothing, just time and writing tests. I think it would be interesting if we want to do test-driven development or behavior-driven development for v1 (i.e. draft the API, write all the tests, and once all tests pass v1 is complete).

@adamhutchings
Copy link
Contributor

I think that is a worthwhile discussion to have, but if we have the basis of testing set up, we can probably close this PR and add more tests as they come (or however else we agree to do it).

@Sophon96
Copy link
Member Author

Sophon96 commented Oct 8, 2023

Blocked #38

@Sophon96 Sophon96 mentioned this pull request Oct 9, 2023
@Sophon96
Copy link
Member Author

Sophon96 commented Oct 11, 2023

TODO before this can be merged

  • Update docs (just README.md for now)
  • Tests for the rest of the code (Model, though not the entirety because of the lack of actual implementation; getters and setters)
  • Add more getters (Matrix needs some, Model too)
  • Review refactored code and breaking API changes introduced in this PR.

@Sophon96
Copy link
Member Author

C API is not tested.

@Sophon96 Sophon96 marked this pull request as ready for review October 15, 2023 16:46
@Sophon96 Sophon96 changed the title Add unit testing framework and tests for existing code Add unit testing framework and tests for existing code (and assorted fixes) Oct 15, 2023
@Sophon96 Sophon96 changed the title Add unit testing framework and tests for existing code (and assorted fixes) Add unit testing framework and tests for existing code (and assorted fixes and breaking API changes) Oct 15, 2023
@Sophon96

This comment was marked as outdated.

@Sophon96
Copy link
Member Author

@adamhutchings @JakeRoggenbuck Ready to review

Copy link
Contributor

@adamhutchings adamhutchings left a comment

Choose a reason for hiding this comment

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

Much appreciated!

REQUIRE(act.f(1) == 1);

REQUIRE(act.df(-1) == 0);
// REQUIRE(act.df(0) == 0); FIXME: is this UB?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is UB because we can just arbitrarily choose a value for the derivative. Like you chose to do, probably better not to test this because the behavior of the derivative at the discontinuity doesn't matter.

@adamhutchings adamhutchings merged commit 96dcdbd into jabacat:main Oct 15, 2023
3 checks passed
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.

3 participants