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

Patch defer.inlineCallbacks to check logcontexts in tests #4205

Merged
merged 3 commits into from Dec 4, 2018

Conversation

3 participants
@richvdh
Copy link
Member

richvdh commented Nov 19, 2018

This is a pretty grim hack, but it helped me find a bunch of the issues fixed in #4204 / #4209 (on which this PR is based, otherwise it would fail CI). Hopefully the intention is clear. If anybody has any ideas for how to do it in a less awful fashion, I'd like to hear.

@richvdh richvdh requested a review from matrix-org/synapse-core Nov 19, 2018

@richvdh richvdh added this to To Do in Backend Core Team via automation Nov 21, 2018

@richvdh richvdh moved this from To Do to Review in Backend Core Team Nov 21, 2018

@richvdh richvdh force-pushed the rav/patch_inline_callbacks branch from c751ac2 to 8a325ae Nov 27, 2018

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Nov 27, 2018

[rebased on develop now that #4204 and #4209 have been merged]

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 27, 2018

Codecov Report

Merging #4205 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4205   +/-   ##
========================================
  Coverage    66.65%   66.65%           
========================================
  Files          299      299           
  Lines        29798    29798           
  Branches      4871     4871           
========================================
  Hits         19863    19863           
  Misses        8501     8501           
  Partials      1434     1434
Impacted Files Coverage Δ
synapse/handlers/user_directory.py 70.42% <0%> (-0.31%) ⬇️
synapse/handlers/presence.py 76.06% <0%> (-0.21%) ⬇️
synapse/state/v1.py 89.14% <0%> (+1.55%) ⬆️

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 944d524...8a325ae. Read the comment docs.

@erikjohnston
Copy link
Member

erikjohnston left a comment

That is pretty awful, but also probably fine.

import tests.patch_inline_callbacks

# attempt to do the patch before we load any synapse code
tests.patch_inline_callbacks.do_patch()

This comment has been minimized.

@erikjohnston

erikjohnston Dec 4, 2018

Member

I wonder if we should print/log that we're replacing inlineCallbacks? Just so that if we accidentally end up importing the tests package there will be some indication that this has happened

This comment has been minimized.

@richvdh

richvdh Dec 4, 2018

Member

could do. I'm unconvinced such a warning would be noticed.

@richvdh richvdh merged commit 48972ce into develop Dec 4, 2018

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Backend Core Team automation moved this from Review to Done - Operations Dec 4, 2018

@richvdh richvdh deleted the rav/patch_inline_callbacks branch Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment