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

BACKPORT: Introduction of Config.invocation_args #6870

Merged
merged 1 commit into from
May 8, 2020

Conversation

fermezz
Copy link

@fermezz fermezz commented Mar 6, 2020

This change was first introduced in #5564 but, logically, only since the 5.1.x version. I'm back-porting it into the 4.6.x branch since many people still use it in the meantime they migrate to Python 3.

I was also able to test this backport to be working successfully locally.

@@ -92,6 +92,7 @@ Evan Kepner
Fabien Zarifian
Fabio Zadrozny
Feng Ma
Fernando Mezzabotta Rey
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if this the thing to do when back-porting things. If it's not, let me know! Just following the steps-to-contribute 🙂

Copy link
Member

Choose a reason for hiding this comment

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

It is definitely the right thing! 👍

@@ -17,9 +17,9 @@ are available on PyPI.

While pytest ``5.0`` will be the new mainstream and development version, until **January 2020**
the pytest core team plans to make bug-fix releases of the pytest ``4.6`` series by
back-porting patches to the ``4.6-maintenance`` branch that affect Python 2 users.
back-porting patches to the ``4.6.x`` branch that affect Python 2 users.
Copy link
Author

@fermezz fermezz Mar 6, 2020

Choose a reason for hiding this comment

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

Since there is not an actual 4.6-maintenance branch, I changed it for what I believe is correct. This should probably be in master as well. I can make that happen if I get confirmation that that's ok.

Copy link
Member

Choose a reason for hiding this comment

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

No need to port to master, we have changed that significantly already: https://docs.pytest.org/en/latest/py27-py34-deprecation.html

@asottile
Copy link
Member

asottile commented Mar 6, 2020

imo this patch extends beyond "bugfix" and probably shouldn't be backported -- curious to hear others thoughts though

@fermezz
Copy link
Author

fermezz commented Mar 6, 2020

Yeah, the way I think about it is that this change, although feature, was specifically introduced to fix a bug in the interaction with pytest-xdist. So while technically it is a feature for pytest, it's a bugfix for the pytest-ecosystem.

That said, I'm open to suggestions. I can maybe find a workaround for my specific use case, but I'm probably not the only one still not being able to update to 5.1 (because of Python 2).

What do you think?

@blueyed
Copy link
Contributor

blueyed commented Mar 7, 2020

@fermezz
FWIW I think it makes sense, given that it is a bugfix after all.

For the Travis config check https://github.com/blueyed/pytest/blob/my-4.6-maintenance/.travis.yml, where it still works.

@fermezz
Copy link
Author

fermezz commented Mar 8, 2020

Thanks @blueyed. I'll give it a shot at fixing that up, although I don't see a lot of differences beyond having fewer jobs to build 🤔.

@fermezz
Copy link
Author

fermezz commented Mar 9, 2020

I have a separate PR to fix CI by migrating it to GitHub actions #6884. I appreciate input there, especially since I'm going to need assistance on modifying the required checks. Also, see #6821

nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 10, 2020
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 10, 2020
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 10, 2020
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Mar 10, 2020
As per comment: pytest-dev#6870 (comment)

Co-authored-by: Daniel Hahler <git@thequod.de>
@nicoddemus
Copy link
Member

imo this patch extends beyond "bugfix" and probably shouldn't be backported -- curious to hear others thoughts though

While I agree that technically it is a (small) new feature, the main reason this was created was for a bug fix in xdist as @fermezz mentioned. Given that this allows to use a more recent pytest-xdist version which fixed a serious config problem, I think we can open an exception. 👍

@@ -0,0 +1 @@
New ``Config.invocation_args`` attribute containing the unchanged arguments passed to ``pytest.main()``.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a remark here:

Suggested change
New ``Config.invocation_args`` attribute containing the unchanged arguments passed to ``pytest.main()``.
New ``Config.invocation_args`` attribute containing the unchanged arguments passed to ``pytest.main()``.
Remark: while this is technically a new feature and according to our `policy <https://docs.pytest.org/en/latest/py27-py34-deprecation.html#what-goes-into-4-6-x-releases>`_ it should not have been backported, we have opened an exception in this particular case because it fixes a serious interaction with ``pytest-xdist``, so it can also be considered a bugfix.

@asottile what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

👍 makes it much more clear that this is one-off and not a greenlight to resume features on 4.6

Copy link
Author

Choose a reason for hiding this comment

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

Remark added, it makes a lot of sense.

Copy link
Member

Choose a reason for hiding this comment

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

Oh just noticed this was not added in the end.

No worries, I will update directly on #7199. 👍

Copy link
Author

Choose a reason for hiding this comment

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

I committed it locally and never pushed 🤦. Sorry about that

Copy link
Member

Choose a reason for hiding this comment

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

No problem 😁

4.6.10 has been released btw! 👍

@fermezz
Copy link
Author

fermezz commented Mar 11, 2020

I added a remark to be clear that this is a one time thing about backporting a feature. We should rebase this branch after #6884 is merged so that we can have a successful travis run before merging it.

@fermezz
Copy link
Author

fermezz commented Apr 13, 2020

Thoughts about this one?

@nicoddemus
Copy link
Member

@fermezz sorry about the delay, we were resolving some internal issues.

Thanks a lot for the PR!

@nicoddemus nicoddemus merged commit ded772b into pytest-dev:4.6.x May 8, 2020
@fermezz
Copy link
Author

fermezz commented May 8, 2020

@nicoddemus I heard, sorry you had to figured that out, but I'm glad y'all are back! And thanks for assisting!!

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