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

fix:Default connection middleware order for Faraday < 1.0 #1129

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Jul 31, 2020

Follow up from #1057
Related to #978

This PR addresses the middleware insertion order for global default connections (e.g. Faraday.get) for Faraday < 1.0.
Functionality is not affect by this change, but a warning emitted by Faraday is now addressed:

WARNING: Unexpected middleware set after the adapter. This won't be supported from Faraday 1.0.

Faraday >= 1.0 is not affected, as it uses a different patching code path.

Testing that warnings won't happen in the future is tricky: because activating an integration has irreversible side-effects on the patched library (Faraday in our case), we can only test Datadog::Contrib::Faraday::Patcher.patch once per RSpec execution. This would require us to perform all our assertions in a single example run, which is not very practical. We also don't have a mechanism to ensure that a specific test runs first during an RSpec run, as successive runs of Datadog::Contrib::Faraday::Patcher.patch will just be a no-op.

There are workarounds for this: initializing a fresh child Ruby process to execute specific tests; separating the patcher test into its own RSpec Rake task. But these are not trivial nor great options, and both come with performance and maintenance burden, so we'll try to address this issue in a systemic way in the future.

@marcotc marcotc added the bug Involves a bug label Jul 31, 2020
@marcotc marcotc requested a review from a team July 31, 2020 19:58
@marcotc marcotc self-assigned this Jul 31, 2020
Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as the patch, things lgtm. 👍

As far as the specs, re:

we'll try to address this issue in a systemic way in the future.

I agree that this is fine for now but is something to address long term. I guess two things that come to mind (just writing this out for posterity, doesn't need to be addressed in this PR) :

  • For background, How do we currently explicitly test against different major versions of a library. Is this basically all done via appraisals?
  • Additionally, do we have any existing tests that check against logging output? If so, Maybe we can make this approach more modular/generalized, and use it the same way analytic_examples is used currently.

@marcotc
Copy link
Member Author

marcotc commented Aug 4, 2020

@ericmustin

For background, How do we currently explicitly test against different major versions of a library. Is this basically all done via appraisals?

Yes, in Faraday's case, we have contrib-old and contrib Appraisal groups in which we test Faraday < 1.0 and latest version, respectively.

Additionally, do we have any existing tests that check against logging output? If so, Maybe we can make this approach more modular/generalized, and use it the same way analytic_examples is used currently.

We do have a few, but all the ones I found are asserting on repeatable code paths, so they don't have the same issue as asserting warnings on the one-off #patch method.

@marcotc marcotc merged commit ebf404f into master Aug 4, 2020
@marcotc marcotc deleted the fix/warning-faraday branch August 4, 2020 18:01
@marcotc marcotc added this to the 0.39.0 milestone Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants