-
Notifications
You must be signed in to change notification settings - Fork 38
Update c driver tests to 3.x and enable deployment #795
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
Update c driver tests to 3.x and enable deployment #795
Conversation
.circleci/config.yml
Outdated
| steps: | ||
| - run: | | ||
| brew install [email protected] | ||
| brew install --skip-post-install [email protected] && brew postinstall [email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it fix, again? I think I saw a similar PR, but I don't remember
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase please - i just deleted this @krishnangovindraj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post install fails for some reason. This doesn't.
But it looks like Joshua fixed it.
| if (check_error_may_print(__FILE__, __LINE__)) goto cleanup; | ||
| options = driver_options_new(false, NULL);; | ||
| if (check_error_may_print(__FILE__, __LINE__)) goto cleanup; | ||
| TypeDBDriver* driver = driver_open_with_description(address, creds, options, DRIVER_LANG); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we discussed with @flyingsilverfin that we want a separate driver_open function for C, which uses DRIVER_LANG implicitly.
This function should be:
- Easily preferred by a user compared to
driver_open_with_description - Easily rejected by a developer of another language (probably with
c_langor something) - Ideally, the only function available for the C devs (not possible,
driver_open_with_descriptionwill always be accessible)
We can rename driver_open_with_description to driver_open_for_swig or something scarier just to be very explicit since we don't really want these descriptions to be compromised. Once done, let's use this function in these tests instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can easily do this - however, currently think this is very low priority!
| if (check_error_may_print(__FILE__, __LINE__)) goto cleanup; | ||
| TypeDBDriver* driver = driver_open_with_description(address, creds, options, DRIVER_LANG); | ||
| cleanup: | ||
| driver_options_drop(options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it noop if there is nothing to drop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think there's a null check. If it's got a non-null garbage value that's probably a segfault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it will be a good practice to reimplement the README example (generally called example or test_example in unit tests) in C, just like for any other language.
Maybe, when it's done, this test will no longer be needed. Sorry that I didn't think about it when replying to the tests-related messages in Discord.
When we have this example test with comments and stuff, we can implement the copy-to-README mechanism we use in other languages (at least the ones that I've updated myself) to have it in README. It's very simple, you can practice with it or ask me to do so.
Everything can be done using LLMs (give this code as the usage reference and give the Rust or any other example test to rewrite) for the baseline, then iterate to cleanup & fix if needed. As always.
@flyingsilverfin wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the convention I now have, as of #792 , is to have test_driver and test_example. The example is where we put examples for usage, and test_driver is where we can put random integration tests. Let's use that convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can write it but I'm unsure how to write it. Probably with a lot of tasty macros.
But can we do that in a different PR? This is basically just to get it published for whoever wants to implement new language drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that doing straightforward operations is okay, macros can be confusing.
I don't mind doing it in a separate PR, but I believe that this process should be a part of the driver preparation, including "for whoever wants to implement new language drivers", as it's the full standardized usage example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example test for a given language (here, C) has to be how you'd expect to use the driver in that language.
That will look nothing like implementation of an FFI driver: see, the C++ driver for example where we don't have the SWIG layer.
Writing it without macros will likely make it some thousand lines long.
|
When we're done here, we may need to update docs. Keep it in mind, please |
flyingsilverfin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase && bring back the README template section for the C deployment which I deleted!
0cb6566 to
570fd60
Compare
9628268 to
ee45401
Compare
Usage and product changes
Update c driver tests to 3.x and enable deployment