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 pypy CI failure #1521

Merged
merged 2 commits into from Feb 13, 2020
Merged

fix pypy CI failure #1521

merged 2 commits into from Feb 13, 2020

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Feb 12, 2020

Fixes CI error seen with other patches.

@ssbarnea ssbarnea changed the title WIP: repair CI Avoid pypy test failure due to undefined printout var Feb 12, 2020
@ssbarnea
Copy link
Member Author

@ssbarnea ssbarnea commented Feb 12, 2020

@gaborbernat While current PR is fixing one bug it does not fix the test failure. I need your help regarding what needs to be done regarding https://dev.azure.com/toxdev/tox/_build/results?buildId=2318&view=ms.vss-test-web.build-test-results-tab&runId=1025542&resultId=100172&paneView=debug

Mainly the test fails because no warning is displayed.

@ssbarnea ssbarnea requested a review from gaborbernat Feb 12, 2020
@ssbarnea ssbarnea marked this pull request as ready for review Feb 12, 2020
@@ -75,7 +75,7 @@ def collect_process(process):
elif status is not None:
outcome = spinner.fail
outcome(env_name)
if print_out and output is not None:
if vars().get("print_out", None) and output is not None:
Copy link
Member

@asottile asottile Feb 12, 2020

Choose a reason for hiding this comment

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

I don't think this is a correct solution, probably just need to move the definition of print_out outside of the try block

Copy link
Member Author

@ssbarnea ssbarnea Feb 12, 2020

Choose a reason for hiding this comment

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

Sure. The hard question is about the missing warning, which causes that specific test to fail.

@gaborbernat
Copy link
Member

@gaborbernat gaborbernat commented Feb 12, 2020

I'm not sure why it fails, would need to debug to understand why it's not triggering 👍

@ssbarnea
Copy link
Member Author

@ssbarnea ssbarnea commented Feb 13, 2020

@gaborbernat @asottile I was able to narrow down the bug, it is CI specific and happens if you do not also have pypy3 installed when trying to run pypy environment. This unique test will fail, but it should be skipped.

It is easy to replicate locally as I didn't have any of the pypy versions installed. I installed pypy and run tox -e pypy, failed like on CI with this single test. After this I installed pypy3 and run the tests again, this time it passed.

I think that test should be change to be skipped when pypy3 executable is not found.

I have doubts that we can easily configure Azure to install both python for this job, see https://github.com/tox-dev/azure-pipelines-template/blob/master/run-tox-env.yml#L91-L94

@gaborbernat
Copy link
Member

@gaborbernat gaborbernat commented Feb 13, 2020

Fixes CI error seen with other patches.
@ssbarnea
Copy link
Member Author

@ssbarnea ssbarnea commented Feb 13, 2020

Maybe you can explain me why https://github.com/pypa/virtualenv/blob/master/azure-pipelines.yml#L102-L106 lines did not also install pypy3. Based on what I read there, that task was supposed to also add the pypy3 near the default pypy2.

Anyway, I made a change to the test to do an xfail when both executable are not found, as that is the only case where the conflict warning is expected to happen.

@ssbarnea ssbarnea requested a review from asottile Feb 13, 2020
@ssbarnea
Copy link
Member Author

@ssbarnea ssbarnea commented Feb 13, 2020

🙀 green! Ship it before it sinks!

Copy link
Member

@gaborbernat gaborbernat left a comment

Thanks, I'm not a fan of the os.system call though. We have our own discovery system we should be using instead of that hack 🤔 how does this even work on non POSIX platforms 🤔

@ssbarnea
Copy link
Member Author

@ssbarnea ssbarnea commented Feb 13, 2020

@gaborbernat Please make a suggestion/edit with the alternative implementation as I am used to that discovery logic. I just want to make CI green ASAP to merge and release #1519 -- as tox itself is affected by the virtualenv20 issues. Since I did not do anything else, and I seen a dozen others around me doing the same.

@gaborbernat
Copy link
Member

@gaborbernat gaborbernat commented Feb 13, 2020

Will address this in a few hours.

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat changed the title Avoid pypy test failure due to undefined printout var fix pypy CI failure Feb 13, 2020
@gaborbernat
Copy link
Member

@gaborbernat gaborbernat commented Feb 13, 2020

Sigh pypy3 is very-very-very slow... 😢

@gaborbernat gaborbernat merged commit d85d2b4 into tox-dev:master Feb 13, 2020
2 checks passed
bmwiedemann added a commit to bmwiedemann/openSUSE that referenced this issue Mar 26, 2020
https://build.opensuse.org/request/show/786809
by user scarabeus_iv + dimstar_suse
- Disable spinner tests as the monkeypatch changed behaviour in pytest

- version update to 3.14.5
  - Add ``--discover`` (fallback to ``TOX_DISCOVER`` environment variable via path separator) to inject python executables
    to try as first step of a discovery - note the executable still needs to match the environment by :user:`gaborbernat`.
    `#1526 <https://github.com/tox-dev/tox/issues/1526>`_
  - Bump minimal six version needed to avoid using one incompatible with newer
    virtualenv. - by :user:`ssbarnea`
    `#1519 <https://github.com/tox-dev/tox/issues/1519>`_
  - Avoid pypy test failure due to undefined printout var. - by :user:`ssbarnea`
    `#1521 <https://github.com/tox-dev/tox/issues/1521>`_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants