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

Fix regression with custom arguments being dropped in non-local executions #491

Merged
merged 9 commits into from Dec 20, 2019

Conversation

ionelmc
Copy link
Member

@ionelmc ionelmc commented Dec 7, 2019

This fixes regression from #448 but lets see if it breaks other stuff.

@nicoddemus
Copy link
Member

Thanks @ionelmc! It seems #448 unintentionlly also threw away pytest-xdist's own options, correct?

Any chance of writing an acceptance test that reproduces the problem to avoid future regressions (like this one in fact)?

We can ignore the py34 failures on Windows for now, it is unrelated and should be fixed/investigated seperately.

@ionelmc
Copy link
Member Author

ionelmc commented Dec 9, 2019

@nicoddemus I extended the chdir test to cover the hook problem.

@ionelmc
Copy link
Member Author

ionelmc commented Dec 9, 2019

Also, I didn't change the CHANGELOG.rst cause I don't really understand what that function does, or what the implications are.

@nicoddemus
Copy link
Member

Also, I didn't change the CHANGELOG.rst cause I don't really understand what that function does, or what the implications are.

No worries, thanks a lot for tackling this, we appreciate it. 👍

I will take a look later.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

giving it a preliminary approval even thou i want to see a additional change, happy sorting out this one

tox.ini Outdated
@@ -11,6 +11,7 @@ deps =
pytestlatest: pytest
pytestmaster: git+https://github.com/pytest-dev/pytest.git@master
pytestfeatures: git+https://github.com/pytest-dev/pytest.git@features
-etesting/foobar
Copy link
Member

Choose a reason for hiding this comment

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

i understand the need for this hack, please add a comment linking this fun hack with the test id

@RonnyPfannschmidt
Copy link
Member

also extra note, i caught a glimpse of the real issue and i recoil thinking about how to make it possible to fix it

the config init keeps giving and giving

@nicoddemus
Copy link
Member

@RonnyPfannschmidt hmm even updating virtualenv didn't help. Any other ideas? I'm fine with removing py34 testing from Windows if we can't figure this out.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus based on the errors, its using a different virtualenv than the one we have gotten installed, due to path issues, i believe its an indication the builds scripts are fundamentally wrong, if we take off py34, we should take it off everywhere

@nicoddemus
Copy link
Member

i believe its an indication the builds scripts are fundamentally wrong, if we take off py34

Which "build scripts" you mean? Note that we test using plain tox, without intermediate scripts.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus i looke in the wrong place, but as far as i can tell we dont update virtualenv

it might help to use tox-venv

@nicoddemus nicoddemus changed the title No idea what I'm doing. Fix regression with custom arguments being dropped in non-local executions Dec 19, 2019
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 again @ionelmc, and sorry for the delay on this.

I've changed the test so it creates the plugin locally instead of installing it via pip -e.

I will rebase, merge, and make a new release once everything passes. 👍

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

3 participants