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

contrib.httplib.patch: Don't overwrite return value #380

Merged
merged 6 commits into from
Jan 8, 2018

Conversation

yoichi
Copy link
Contributor

@yoichi yoichi commented Nov 16, 2017

Fix #323

@yoichi yoichi changed the title contrib.http.lib.patch: Don't overwrite return value contrib.httplib.patch: Don't overwrite return value Nov 16, 2017
@palazzem palazzem modified the milestones: 0.11.0, 0.10.1 Nov 16, 2017
@palazzem palazzem self-requested a review November 16, 2017 14:15
@palazzem
Copy link

Thanks for this contribution @yoichi ! We'll review and ship that one with a minor bugfix release if the 0.11.0 takes more time.

Copy link

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution @yoichi ! going to merge as soon the CI is green!

@yoichi
Copy link
Contributor Author

yoichi commented Nov 17, 2017

ci/circleci: mysqlconnector — Your tests failed on CircleCI

It's not caused by my change.

tests/wait-for-services.py:

    if len(sys.argv) > 2:
        for service in sys.argv[1:]:
            check_functions[service]()
    else:
        print("usage: python {} SERVICE_NAME".format(sys.argv[0]))
        sys.exit(1)

condition should be len(sys.argv) >= 2.

@yoichi
Copy link
Contributor Author

yoichi commented Nov 28, 2017

I think CircleCI test failure is not caused by the change of this PR.
It is caused by a timing issue of existing test code:
https://github.com/DataDog/dd-trace-py/blob/master/tests/test_integration.py#L197
sleep a little more (e.g. 1.1 seconds) will reduce the frequency of occurance.

Regards,

======================================================================
FAIL: test_worker_http_error_logging (tests.test_integration.TestWorkers)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/project/tests/test_integration.py", line 202, in test_worker_http_error_logging
    eq_(len(logged_errors), 1)
AssertionError: 0 != 1

@palazzem
Copy link

@yoichi ok let me see if we can make it pass. In general I don't want to rely on timings because it makes the test flaky. Let me schedule some work on our tests otherwise it would be difficult for you to contribute to this repository.

Thanks for spotting that by the way!

@palazzem
Copy link

palazzem commented Jan 8, 2018

@yoichi sorry for late response! everything is good and we're going to merge that PR now. Thank you very much for your contribution!

/cc @thehesiod

@palazzem palazzem merged commit 62be630 into DataDog:master Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants