-
Notifications
You must be signed in to change notification settings - Fork 234
Fix regression with custom arguments being dropped in non-local executions #491
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
Conversation
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. |
@nicoddemus I extended the chdir test to cover the hook problem. |
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. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
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 |
@RonnyPfannschmidt hmm even updating |
@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 |
Which "build scripts" you mean? Note that we test using plain tox, without intermediate scripts. |
@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 |
There was a problem hiding this 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. 👍
This fixes regression from #448 but lets see if it breaks other stuff.