Force xunit2 for pytest 6.0 when legacy mode is enabled (take III)#471
Force xunit2 for pytest 6.0 when legacy mode is enabled (take III)#471
Conversation
nuclearsandwich
left a comment
There was a problem hiding this comment.
I think some variables might be incorrectly or unclearly named.
|
Can I make a suggestion here that might negate the need for this PR completely? In order to have clean test runs, we need to install a pytest.ini file in all of the relevant repos. Otherwise, pytest throws this xunit2 warning. While this works around it, it still causes these warnings to show up when running locally. I've been slowly adding pytest.ini to the relevant places as I have noticed them, but I might suggest that now would be a good opportunity to finish that work off. Then we can remove this workaround from CI completely. Thoughts? |
As long as pytest 3.3.2 in Bionic supports the configuration option I'm completely totally 100% in favor of that approach. I was reviewing under the assumption that adding the appropriate configuration directly to affected repositories was off the table, otherwise why work around it. If we can't backport the config option to Bionic it will affect Dashing and Eloquent CI runs on ci.ros2.org. |
|
Let me see what it will take to get rid of it for master. Then I'll look at the options for backporting to bionic. |
|
@j-rivero if there is some reason we don't want to or can't make these changes at the source level can you add them to the PR description and let us know. |
|
All right, it looks doable. It will take 19 PRs (that's just for master), plus probably an equivalent amount for Foxy, Eloquent and Dashing, but I think that is a better way forward. Once I get all of the PRs merged to master, I'll do a CI run with this workaround removed completely and see what it looks like. |
|
(also, as far as I know this should work fine for Ubuntu 18.04, so eloquent and dashing should be fine) |
I mentioned that briefly at the beginning of all the PRs serie. Nothing against it but that fix relies on no changes in current code with respect to how the format is configured in pytest.ini files and the same for new packages. If one of them is miss configured the whole build fails. In order to be sure that the build is not going to fail in any case we can keep these changes in place together with fixes in source code level. |
Why would the build fail? It only results in a warning is a package doesn't set the option and the workaround in CI is removed. |
|
If all packages have the option defined we should revert the working in CI since there is a) no need for it and b) we want to be notified that any new package doesn't have option set (since a user building from source will experience the same). |
Would not be the same situation that leaded to #393? The |
|
@j-rivero Can you please address the pending feedback so we can get this PR merged. As a follow up we want to remove the workaround to generate custom |
Co-authored-by: Steven! Ragnarök <nuclearsandwich@users.noreply.github.com>
Co-authored-by: Steven! Ragnarök <nuclearsandwich@users.noreply.github.com>
Co-authored-by: Steven! Ragnarök <nuclearsandwich@users.noreply.github.com>
ready. A latest run to check these latest changes: |
probably would need a rebase or other alternatives to get changes from master in order to work. |
|
@j-rivero What is the status on this ticket? |
ready to merge as far as I can tell. I've merge master to get the final test build: |
dirk-thomas
left a comment
There was a problem hiding this comment.
Beside the inline comment this LGTM.
|
After merge with default and solve the quoting problem, last test run |
| # format if it is not present | ||
| # check if packages have a pytest.ini file that doesn't choose junit_family=xunit2 | ||
| # and patch configuration if needed to force the xunit2 value | ||
| pytest_6_or_greater = job.run([ |
There was a problem hiding this comment.
Looks like if this check fails, it fails the build (for example).
Part III of my particular drama to implement correctly the check for pytest 6.0. The original implementation was in #415 and fixed after in #469. This last fix wrongly assumed that the pytest version check was happening inside the virtualenv. It was not detected by the CI jobs in the PR since the build node that run it happen to have pytest installed in the base system.
This PR tries to run the check inside the virtualenv on the testing job. First commit db9e19c back to enable the reverted code from yesterday, second commit are the real changes 8b6b63d I made to run on virtualenv.
Tested: