-
Notifications
You must be signed in to change notification settings - Fork 78
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
Use async await #45
Use async await #45
Conversation
I think running async stuff in drop is UB (tests fail with SIGILL which indicates UB). I think we need to wrap the test functions manually for teardown. For example like this: https://medium.com/@ericdreichert/test-setup-and-teardown-in-rust-without-a-framework-ba32d97aa5ab |
Thanks for this PR! My test harness was based on the post you linked. Cleanup might be omittable but will create artefacts while locally developing. Ci will create a new influxdb each run. |
1.39 due to async / await
I think by now this PR is best reviewed commit-by-commit. I added a commit that indicates the minimum required rust version. Tests still need fixing. |
I think the tests are fine now. The failure probably occurs because Travis does not expose secrets to PRs to avoid leaking them to untrusted code. Let's wait until reqwest makes the next release. Then I'll update this PR and we can merge this to see if the tests are truly okay. |
Sorry, for noting just now, there's already a better test harness used in #23: https://github.com/Empty2k12/influxdb-rust/pull/23/files#diff-16b1336c25d7d5d15e6e55f3e72fef07R35-R47 Is yours compatible, so I can easily fix the other PR? |
That one is clearly more concise, but currently error prone. It is very important to catch unwinds in test harnesses, so that the teardown is still executed even when the tests panic (and thus fail). Since all the cleanups do is create and delete tables, the one from this PR can be adapted easily. My suggestion: keep the one from this PR, because it integrates better with modern tokio (no |
Great, I appreciate your work on this PR. Once you update |
If you're fine with not-releasing this library for a while we can also do the merge now and wait for the new reqwest release in-tree. Then we have time to update the other PR. |
Or you merge the other PR first, we fix the test harness, and we rebase this one. |
reqwest 0.10.0 was just released (4hrs ago) with async/await support. Would be great if this could be wrapped up :) |
Thanks for the heads-up! I‘ll be doing the adoption in the first few days of next year. |
Great, thanks! And a happy new year :)On 30 Dec 2019 23:55, Moritz Gunz <notifications@github.com> wrote:Thanks for the heads-up! I‘ll be doing the adoption in the first few days of next year.
—You are receiving this because you commented.Reply to this email directly, view it on GitHub, or unsubscribe.
|
@Empty2k12 reqwest has been updated. Please review. :) |
|
2e1c30f
to
e28299d
Compare
@Empty2k12 Tests green! Please review. |
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.
LGTM 💚 I apprechiate your efforts to make this library ready for 2020!
I will release this when I'm back in Aachen, FlixTrain Wifi is not good enough for that.
The changes speak for themselves. Let's wait until reqwest gets 0.10 released until we merge this. The integration tests also need fixing.
In a follow-up PR I will optimize the usage of reqwest's HTTP client.