-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: master
Are you sure you want to change the base?
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_lang
or something) - Ideally, the only function available for the C devs (not possible,
driver_open_with_description
will 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 (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 |
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