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

Adding tests to at() #419

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Adding tests to at() #419

merged 4 commits into from
Apr 3, 2024

Conversation

Neko-Box-Coder
Copy link
Contributor

@Neko-Box-Coder Neko-Box-Coder commented Apr 3, 2024

Trying to add tests to ConstImpl at() const in node.hpp for #411

I have no idea if what I am doing is correct or not. And there's a TODO on line 1029 since I am not sure what to replace it with.
If this is totally wrong 😅 , I can close this PR.

I doubt I can help much with adding tests with my limited knowledge on the code base and test framework.
However, I can take a look at the whole PR and test it when it is finalized.

@biojppm
Copy link
Owner

biojppm commented Apr 3, 2024

Nice, this is good, saves me some work and helps!

The only point is that verify_assertion() should be really the (non-existent) verify_error(), which in turn instead of ExpectError::check_assertion() would call ExpectError::do_check() with the same syntax. This is because we want to verify that those calls result in an error callback regardless of RYML_USE_ASSERT; verify_assertion() checks for the former.

@Neko-Box-Coder
Copy link
Contributor Author

Neko-Box-Coder commented Apr 3, 2024

Thanks for the feedback, I have added verify_error() to be used for at(). Makes sense that at() should always check (and callback) for any invalid inputs.

Do I need to add anything else? And what do I replace the capacity() with?

test/test_case.hpp Outdated Show resolved Hide resolved
test/test_tree.cpp Outdated Show resolved Hide resolved
test/test_tree.cpp Show resolved Hide resolved
@biojppm
Copy link
Owner

biojppm commented Apr 3, 2024

Thanks! Don't worry about the capacity. I will dig into it.

@biojppm
Copy link
Owner

biojppm commented Apr 3, 2024

Thanks, it's good and it saved me some time!

@biojppm biojppm merged commit 974a34b into biojppm:fix/389_noexcept Apr 3, 2024
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