Skip to content

Simplify the logic run for pytest version inside the venv#501

Closed
j-rivero wants to merge 3 commits intomasterfrom
shell_pytest6
Closed

Simplify the logic run for pytest version inside the venv#501
j-rivero wants to merge 3 commits intomasterfrom
shell_pytest6

Conversation

@j-rivero
Copy link
Copy Markdown

@j-rivero j-rivero commented Aug 7, 2020

Mostly inspired by this comment of @dirk-thomas #498 (comment). Should help to ix the regressions found in #498.

Implementation: print the pytest version inside the venv using job.run and move the version check logic outside of the venv.

Tested with ament_package on Foxy in:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@j-rivero j-rivero requested a review from jacobperron August 7, 2020 18:17
# check_output capabilities
pytest_6_version_file = os.path.join(args.buildspace, 'pytest6_version_result.txt')
job.run(["%s -c 'import pytest; print(pytest.__version__)' > %s" % (job.python, pytest_6_version_file)])
pytest_6_or_greater = False
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line is unnecessary.

Copy link
Copy Markdown
Author

@j-rivero j-rivero Aug 7, 2020

Choose a reason for hiding this comment

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

if we define the variable only inside the with clause, could it be used outside of that scope?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes.

@j-rivero
Copy link
Copy Markdown
Author

j-rivero commented Aug 7, 2020

Please do not merge, using foxy ros2 repos file is failing:

  • Linux Build Status

Looking into this now.

@j-rivero
Copy link
Copy Markdown
Author

j-rivero commented Aug 7, 2020

This is confusing me: the test display to be using pytest-6.0.1, I've verified that this information is being generated fine in this PR and goes to the temporary file. Some lines after this message appear PytestDeprecationWarning: The 'junit_family' default value will change to 'xunit2' in pytest 6.0. See: like if it was not using pytest6. Or do I am missing something obvious?

�[1m============================= test session starts ==============================�[0m
platform linux -- Python 3.8.2, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
cachedir: /home/jenkins-agent/workspace/ci_linux/ws/build/ament_package/.pytest_cache
rootdir: /home/jenkins-agent/workspace/ci_linux/ws/src/ament/ament_package, configfile: pytest.ini
plugins: colcon-core-0.6.0, mock-3.2.0, repeat-0.8.0, rerunfailures-9.0, cov-2.10.0
�[1mcollecting ... �[0m�[1m
collected 1 item                                                               �[0m

test/test_flake8.py �[32m.�[0m�[33m                                                    [100%]�[0m

�[33m=============================== warnings summary ===============================�[0m
/home/jenkins-agent/workspace/ci_linux/venv/lib/python3.8/site-packages/_pytest/junitxml.py:446
  /home/jenkins-agent/workspace/ci_linux/venv/lib/python3.8/site-packages/_pytest/junitxml.py:446: PytestDeprecationWarning: The 'junit_family' default value will change to 'xunit2' in pytest 6.0. See:
    https://docs.pytest.org/en/stable/deprecations.html#junit-family-default-value-change-to-xunit2
  for more information.
    _issue_warning_captured(deprecated.JUNIT_XML_DEFAULT_FAMILY, config.hook, 2)

@j-rivero
Copy link
Copy Markdown
Author

j-rivero commented Aug 7, 2020

Or do I am missing something obvious?

Don't have more time to investigate what's going on with the pytest enviroment, an alternative to these changes that seems not to fix the problem is pr #502

@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented Aug 7, 2020

It does seem odd. Locally, I can reproduce the test warning with pytest 6.0.1 building ament_package foxy branch.
Maybe we need to make the patch to the pytest.ini regardless?

@jacobperron
Copy link
Copy Markdown
Member

After some reading and experimentation, I think we should expect the deprecation warnings to appear if junit_family is not explicitly set (in the pytest.ini or otherwise).

The problem here is that we're assuming if junit_family is set to "legacy", then we don't have to patch the pytest.ini file; however, we'll still get a deprecation warning if it is not set at all.

Unless there's some reason we shouldn't always set junit_unit=xunit2 for Dashing, Eloquent, and Foxy, I think something like #502 is a cleaner solution. Furthermore, I don't think we need to patch the pytest.ini file, and can instead set junit_family from the command-line (see c6b5dc7)

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.

3 participants