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

Backport PRs #12154, #12294, #12297, #12316, #13159 & #13205 to fix multiple tests issues #13181

Merged

Conversation

Projects
None yet
9 participants
@ArchangeGabriel
Copy link

ArchangeGabriel commented Jan 14, 2019

Once merged, a follow-up PR should revert d7e3789 as was done in #13159 for v3.0.x.

My only fear is that bumping pytest requirement could be seen as an issue (based on my interpretation on #13159 (comment)).

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 14, 2019

appveyor CI fails because of pytest-rerunfailures 6.0. I guess this is a missing constraints upadte in d7e3789 (travis was changed but not appveyor). Should I add it here? And then revert in the follow-up PR?

Travis CIs fail with sh: 0: Can't open /etc/init.d/xvfb, not sure if related.

@tacaswell tacaswell added this to the v2.2.4 milestone Jan 14, 2019

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 14, 2019

My concern about bumping the pytest version requirement on the 2.2.x branch is that we are going to be caught in this trap where either the slower distributions will not have a new enough version of pytest or the faster distributions will not have an old enough version (pytest 3.6 is only from May 2018).

Could you do all of the backporting / reverting in this PR? I think that makes more sense than a series of PRs.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 14, 2019

Sure, I’ll push the revert in a second commit.

Are you aware of distributions still updating matplotlib (even if we are talking patch version here) but not pytest?

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 14, 2019

Pushed. Note that in #12879 (comment), @timhoffm also stated that fixing this in 2.2.x might not be wanted, not sure if this is for the same reason.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 14, 2019

AppVeyor is now failing with numerous E TypeError: cannot concatenate 'str' and 'MarkDecorator' objects, which I already started to investigate in #13182.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

Errors from https://ci.appveyor.com/project/matplotlib/matplotlib/builds/21606888/job/6jl1g3mg6jn1j2ol?fullLog=true are gone with the latest commit (btw, I’m not sure about the coding style in the ps test). However, the TypeError: cannot concatenate 'str' and 'MarkDecorator' objects failures (1225 over 1229 FAILURES) are still there, as well as the four remaining ones, TypeError: coercing to Unicode: need string or buffer, MarkDecorator found.

I’ll keep investigating, but that is already going beyond my knowledge, so help would be appreciated.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

I guess the solution is to be deduced from https://docs.pytest.org/en/latest/mark.html#marker-revamp-and-iteration, but having never manipulated such things I’m a bit lost…

Backporting and testing is easy, fixing is something else. :p

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

pytest-dev/pytest#3576 might also be related.

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 16, 2019

attn @sandrotosi is this going to be a problem of debian?

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

(Might be worst noting that in the latest v2.2.x commit, all tests failing here are skipped)

Also I still don’t understand the Travis failure.

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 16, 2019

This looks like the thing we use to have an xserver on travis stopped working...

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 16, 2019

@ArchangeGabriel Be aware I added some commits (it seemed faster to just make the edits via the GH ui than to ask you to make the changes ), hope you do not mind.

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 16, 2019

ok, that seems to have gotten the python3 test running again, but py27 is failing inside of pytest startup...

@dstansby dstansby added the Testing label Jan 16, 2019

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

@tacaswell Yes I’ve seen, no issue with that, you are very welcome! ;)

So Python 3.4 is working, Python 3.6 & 3.7 are failing with the same error as in #13182. AppVeyor is not working because the lack of tools to compare images makes it try to skip 1229 tests, and the skipping code is what is failing with newer pytest on Py2.

Regarding Debian, it should not be an issue: Stretch has matplotlib 2.0.0, while Buster has matplotlib2 2.2.3 but pytest 3.10.1 (https://packages.debian.org/source/matplotlib and https://packages.debian.org/source/pytest).

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

(I’m investigating the py2 issue, already found pytest-dev/pytest#3889, so likely have to go an higher pytest version)

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

Yup, fixed in 3.6.1: pytest-dev/pytest@b5a94d8

I’ll push a small commit to update to this version at least.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

OK, so now need to re-pin down rerunfailures. I guess <6 will do, checking and pushing.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

Wow, py2 actually passed in Travis. \o/ I’ll have a look in version differences between the different passing/failing builds, to see whether they are some more changes we should catch or if there is a real issue in pytest.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

Small note: the working Py3.4 TravisCI is using pytest 3.8.2, while the failing Py3.6/3.7 are using pytest 4.1.1. Looking at the pytest changelog, I don’t see anything outstanding, but I would bet on pytest 4.1 being the culprit (and since I did not see anything relevant in the changes, I would say it’s a pytest bug).

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 16, 2019

OK, got it. We need to backport #12316 (mainly dbac64f and 46d3860, but others one are interesting too). I’ll do it tomorrow if needed, but if someone with appropriate rights can run meseeksdev in the meantime so that we can at best win some time and at worst give me the appropriate way of backporting a multi-commit PR, that would be very welcomed.

@ArchangeGabriel ArchangeGabriel force-pushed the ArchangeGabriel:auto-backport-of-pr-12154-on-v2.2.x branch from de44136 to cc1c6a8 Jan 17, 2019

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 17, 2019

CI went OK as expected. I’ve fixed my pytest>=3.6.1 commit, and also backported #12294 (but the backport was automatic, so I didn’t take care of the commit message).

I think this PR is now up for review/comments, especially if you want me to change some commit messages or coding style for manual changes.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 18, 2019

@NelleV Not sure why you cycled, the CI gave the same result as just before. MacOS on Travis is broken trying to compile half of the universe before being able to install, so…

@QuLogic

This comment has been minimized.

Copy link
Member

QuLogic commented Jan 18, 2019

It might be necessary to backport #12615 to fix macOS builds.

@QuLogic

This comment has been minimized.

Copy link
Member

QuLogic commented Jan 18, 2019

Though it seems to be ffmpeg that fails, so maybe not.

CI: don't install ffmpeg on OSX on travis
It seems to be timing out the build (because it is falling back to building ffmpeg from scratch?!).
@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 19, 2019

we are not seeing this issue in master because we are trying to install a non-existing tap (Error: No available formula with the name "font-wenquanyi-zen-hei" ) so this whole line is not run and we do not try to install ffmpeg.

@NelleV

This comment has been minimized.

Copy link
Member

NelleV commented Jan 19, 2019

@ArchangeGabriel I cycle when patches merged in the branch may (or may not) fix the test issues. I don't necessarly try to deeply look at the whether the failed tests on the branch correspond to the tests fixed in a previous patch: it would take too much of our time to manually check this while restarting the CI only takes machine time.

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 20, 2019

@NelleV I see, makes sense. :)

So we are now passing with macOS too. What are the next steps for this to get merged?

@NelleV

NelleV approved these changes Jan 21, 2019

Copy link
Member

NelleV left a comment

Thanks!

@NelleV

This comment has been minimized.

Copy link
Member

NelleV commented Jan 21, 2019

@ArchangeGabriel You bug people until they review your pull request. The next core dev' that approves can merge it, so hopefully it should be fast!

@sandrotosi

This comment has been minimized.

Copy link
Contributor

sandrotosi commented Jan 22, 2019

@tacaswell this should be fine for Debian - thanks for checking!

@sandrotosi

This comment has been minimized.

Copy link
Contributor

sandrotosi commented Jan 22, 2019

also, small notice, we're about to start the freeze phase (ie no more new packages) for the next stable Debian release, so if you're planning on releasing mpl 2.x and 3.x this would be a good time :)

@NelleV

This comment has been minimized.

Copy link
Member

NelleV commented Jan 22, 2019

😬

@ArchangeGabriel ArchangeGabriel changed the title Backport PR #12154: Avoid triggering deprecation warnings with pytest 3.8. Backport PRs #12154, #12294, #12297, #12316, #13159 & #13205 to fix multiple tests issues Jan 22, 2019

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 22, 2019

I’ve updated the title to reflect more correctly what’s inside. ;)

@QuLogic QuLogic merged commit 4a5064c into matplotlib:v2.2.x Jan 22, 2019

7 checks passed

ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/patch 93.1% of diff hit (target 50%)
Details
codecov/project/library 67.69% (target 50%)
Details
codecov/project/tests Absolute coverage decreased by -0.06% but relative coverage increased by +1.24% compared to 8482916
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@QuLogic

This comment has been minimized.

Copy link
Member

QuLogic commented Jan 22, 2019

Congrats on getting your first PR into Matplotlib!

@ArchangeGabriel

This comment has been minimized.

Copy link
Author

ArchangeGabriel commented Jan 22, 2019

Thanks! :)

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Jan 22, 2019

@sandrotosi may try to do bug fix releases for 3.0.x and 2.2.x by end end of January.

@shoyer

This comment has been minimized.

Copy link

shoyer commented Mar 14, 2019

Was this ever backported into 3.0.x?

I see the changes on the 2.2.x branch but not 3.0.x, e.g., in
https://github.com/matplotlib/matplotlib/blob/v3.0.x/lib/matplotlib/testing/conftest.py

@tacaswell

This comment has been minimized.

Copy link
Member

tacaswell commented Mar 14, 2019

No, we missed it on accident (see #12154 (comment) ).

Given that we are going to do a 3.1 rc, mixed on if we should do a 3.0.4 to put this in...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.