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

Rework handling of cancelled 'yield' in fixtures #83

Merged
merged 5 commits into from
Jun 11, 2019

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Jun 10, 2019

  • Always report a failure if a fixture cancels the test, even if the
    fixture doesn't then raise an exception
  • Inside fixtures, yield can now raise trio.Cancelled

Closes: #75, #77

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #83 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   99.78%   99.79%   +<.01%     
==========================================
  Files          19       19              
  Lines         476      498      +22     
  Branches       41       43       +2     
==========================================
+ Hits          475      497      +22     
  Misses          1        1
Impacted Files Coverage Δ
pytest_trio/_tests/test_fixture_ordering.py 100% <100%> (ø) ⬆️
pytest_trio/_tests/test_fixture_mistakes.py 100% <100%> (ø) ⬆️
pytest_trio/plugin.py 99.5% <100%> (+0.02%) ⬆️

@njsmith
Copy link
Member Author

njsmith commented Jun 11, 2019

@belm0 @touilleMan thanks for the 👍s! Now, who's going to hit merge? Our contributing docs say that people shouldn't merge their own PRs, so I guess it shouldn't be me...

@belm0
Copy link
Member

belm0 commented Jun 11, 2019

@njsmith thanks for raising, I wasn't familiar with that policy.

In most projects I've been on, PR author merges after required approvals. It's because PR author knows the context best-- e.g. there may be a last minute correction or addition; the merge may need to be timed relative to others, etc.

@belm0
Copy link
Member

belm0 commented Jun 11, 2019

... and ultimately person hitting merge button is responsible for build our runtime breakage. It seems like too much responsibility for a reviewer.

@belm0
Copy link
Member

belm0 commented Jun 11, 2019

Another point I realized as I was going to merge this:

There is also decision about squashing, which really depends on whether commits were logically grouped or ad-hoc. It's best left to PR author. On my PR's I'd want that option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_ctx.crashed() should always make the test end up with crash
3 participants