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 rust-ffi #3671

Closed
wants to merge 1 commit into from

Conversation

gilligan
Copy link
Contributor

@gilligan gilligan commented Jun 8, 2020

Adds very simple tests for String and Result from rust-ffi.h

Note: The Result test case for Err values should be adjusted such
that it carries an actual exception. I failed to get that done however.

@edolstra Maybe you know how to do this.

@gilligan gilligan mentioned this pull request Jun 8, 2020
16 tasks
@Ericson2314
Copy link
Member

Looking forward to this :). I'll add some more in #3450 once this is merged.

@edolstra
Copy link
Member

I feel that because the Rust API is still very much a work in progress, it's premature to add tests at this point. (E.g. if we switch to cxx it will invalidate all these tests.)

@gilligan
Copy link
Contributor Author

@edolstra i would say that is maybe even more reason to do so. The idea behind unit tests is not to forever encode the way something is supposed to work without ever changing. Instead it’s supposed to make it easier to do the changes i’d say.

If there are changes that will land sometime soon that will break this tests that’s great. That means the tests will be fixed and adjusted and also that they cover relevant code.

@Ericson2314 Ericson2314 mentioned this pull request Jun 11, 2020
@gilligan
Copy link
Contributor Author

@edolstra I added a short FAQ over at #3622 which also touches on "unit tests versus functional tests" i.e the topic you are raising here.

@gilligan
Copy link
Contributor Author

Considering #3702 i am going to close this.

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.

None yet

3 participants