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

[4.6.x][backport of PR #6337] Make 'S' and 'F' aliases to 's' and 'f' respectively #7314

Merged
merged 2 commits into from
Jun 4, 2020

Conversation

webknjaz
Copy link
Member

@webknjaz webknjaz commented Jun 3, 2020

This is a backport of #6337 (by @nicoddemus) with an extra change
note on top of it. It fixes #7310 and supersedes closes #7311 (as an
alternative to a smaller patch as suggested by @RonnyPfannschmidt).

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

    Write sentences in the past or present tense, examples:

    • Improved verbose diff output with sequences.
    • Terminal summary statistics now use multiple colors.

    Also make sure to end the sentence with a ..

  • Add yourself to AUTHORS in alphabetical order.

Sorry, something went wrong.

@webknjaz
Copy link
Member Author

webknjaz commented Jun 3, 2020

Alternative PR: #7311.

@webknjaz
Copy link
Member Author

webknjaz commented Jun 3, 2020

N.B. Because there was a conflict during cherry-pick, I had to also absorb a small change related to the error outcome that is present on master but not in 4.6.x.

@@ -0,0 +1,3 @@
Fix summary entries appearing twice when ``f/F`` and ``s/S`` report chars were used at the same time in the ``-r`` command-line option (for example ``-rFf``).
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a part of backport

@@ -0,0 +1,9 @@
Fix ``UnboundLocalError: local variable 'letter' referenced before
Copy link
Member Author

Choose a reason for hiding this comment

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

This is new

Comment on lines +193 to +195
if report.when in ("collect", "setup", "teardown") and outcome == "failed":
outcome = "error"
letter = "E"
Copy link
Member Author

Choose a reason for hiding this comment

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

This got absorbed from the master but wasn't a part of the original PR.

@asottile
Copy link
Member

asottile commented Jun 3, 2020

@webknjaz
Copy link
Member Author

webknjaz commented Jun 3, 2020

@asottile oh, I didn't know. What do you want me to do exactly? Should I redo it using -x? I just targeted the commits from the branch that's in the PR.

@nicoddemus
Copy link
Member

Hi @webknjaz,

Our procedure is to use:

git co -b backport-6337 upstream/4.6.x
git cherry-pick -m1 ecd1e43

I just did that and got some conflicts in terminal.py, which I suppose is the same one you got. With this procedure we get a nice clean commit with all the changes related to that.

Could you try that and see if it works for you?

@webknjaz

This comment has been minimized.

@webknjaz
Copy link
Member Author

webknjaz commented Jun 3, 2020

Also, do I have to follow that branch convention? It doesn't seem important for forks.

nicoddemus and others added 2 commits June 3, 2020 22:33

Unverified

This user has not yet uploaded their public signing key.
(cherry picked from commit ecd1e43)
@webknjaz webknjaz force-pushed the backports/pr-6337 branch from 80ee5d9 to 1c465bd Compare June 3, 2020 20:36
@webknjaz
Copy link
Member Author

webknjaz commented Jun 3, 2020

Ah, I didn't realize that cherry-picking merge commits already does the squash. FWIW I've just force-pushed that as suggested.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks, we appreciate it!

@nicoddemus
Copy link
Member

Also, do I have to follow that branch convention? It doesn't seem important for forks.

No you are right, it was just a matter of suggesting something given we have a guide. 😁

@asottile asottile merged commit 5644437 into pytest-dev:4.6.x Jun 4, 2020
@RonnyPfannschmidt
Copy link
Member

@webknjaz cherry picking a merge commit will choose the diff to the given parent

@webknjaz
Copy link
Member Author

webknjaz commented Jun 4, 2020

@RonnyPfannschmidt yeah, I figured. For some I just thought that it'd cherry-pick series of commits from the incoming branch.

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.

None yet

4 participants