Codecov Report

❗ No coverage uploaded for pull request base (master@afe0232). Click here to learn what that means.
The diff coverage is 60%.

@@            Coverage Diff            @@
##             master     #203   +/-   ##
=========================================
  Coverage          ?   76.09%           
=========================================
  Files             ?       29           
  Lines             ?     2389           
  Branches          ?      409           
=========================================
  Hits              ?     1818           
  Misses            ?      423           
  Partials          ?      148

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afe0232...c50e82f. Read the comment docs.

@mlennon-lumere I pushed what I have right now but I probably won't get around to it this week. As stated in the linked issue the problem is that none of this is properly tested.

I think there could be tests added here. But the reason I'd prefer this approach instead of what's in #202 is that with the approach in #202, you'd have to have a case to handle each and every thing that psycopg2 could throw at you.

For example, sql.Placeholder instances, or sql.Literal instances, or other classes that psycopg2 hasn't even developed yet.

By standing on their shoulders with mogrify, sentry-python should be able to get their updates and keep feature-parity for much less effort

Yes I closed #202 because I think this is the superior approach. It's still harder to test though. A test like "assert that our integration calls mogrify" without actually having a real psycopg cursor isn't good enough I think, considering all the bugs we have ran into

Is this still being worked on? Cheers

Sorry, we've been really busy with non-sdk related stuff recently. I'll try to make some room for this. As far as I remember I was unhappy with the tests.

We're hoping to get these changes upstream, so we can stop relying on our fork. Do you need something else to get this done, or is simple patience the way to go?

Thanks!

I really want to merge this with decent tests only. Right now we don't have any other customer/user complaining about this so it's kind of low-priority

What sort of unit tests do you believe are missing? I'm hoping to see this get in, our org needs it to be able to keep using sentry.

Looking at the tests output is difficult, as many of the test runs on travis failed because of hitting an API rate limit. Can you guide as to what you'd like to see?

Thanks

@mlennon-lumere I merged master into this branch, I hope the errors are now clearer. We recently added a CONTRIBUTING.md which should make it easier for you to set up a local dev env.

This has been a long time ago, but I believe the issue here was that we configured a sqlite database while testing postgres specific stuff. This has already caused some issues in the past, but so far we've been able to get away with it even though the query crashed halfway through.

I'm not sure what's going on with the celery tests breaking - thanks for having a look and fixing the bytestring issues, postgres config

Seems unrelated. Thanks for the PR!

Thanks for merging - when can we expect a release?

Thanks again

0.7.8 is out!