8000 MNT: Make sure AppVeyor fails if tests fail by dopplershift · Pull Request #9773 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

MNT: Make sure AppVeyor fails if tests fail #9773

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 23, 2017
Merged

MNT: Make sure AppVeyor fails if tests fail #9773

merged 5 commits into from
Nov 23, 2017

Conversation

dopplershift
Copy link
Contributor
@dopplershift dopplershift commented Nov 13, 2017 •
edited
Loading

Need to make sure pytest is the last thing in test_script, so move some
of these commands elsewhere as appropriate. Also remove duplicated
command to visualize test results.

Closes #9719 .

@dopplershift dopplershift added this to the v2.1.1 milestone Nov 13, 2017
@dopplershift
Copy link
Contributor Author

So my guess at the problem seems to be wrong. Next up: is matplotlib.test somehow wrong? Find out on our next exciting episode...

@dstansby dstansby added topic: testing Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels Nov 14, 2017
@dstansby
Copy link
Member

I've just checked, and in tests.py the right retcode (1) is being passed to sys.exit: https://ci.appveyor.com/project/dstansby/matplotlib/build/1.0.3/job/adnrjcr682b5wc8h#L1930

@dstansby
Copy link
Member

Adding in the if ($LastExitCode -ne 0) { $host.SetShouldExit($LastExitCode) } appveyor script seems to work: https://ci.appveyor.com/project/dstansby/matplotlib/build/1.0.4/job/t0d6sntdir5hwb8w#L1931

@dopplershift
MNT: Clean up AppVeyor a bit
5572869 
Upload coverage as part of post processing rather than test script. Also
remove duplicated command to visualize test results.
@dopplershift
Copy link
Contributor Author

Not sure why that's necessary for us here when I've never needed it before on other projects...

Then again we're the only one I've used that has a magic script rather than running pytest. 😈

@dstansby
Copy link
Member

No idea either... Appveyor has always seemed a bit mysterious to me. Now all we need to do is fix the actual errors 😛

@Kojoley
Copy link
Member
Kojoley commented Nov 14, 2017

I have a guess, maybe somehow set errorlevel= breaks the thing? Try to replace it with cmd /c "exit /b 0"

@dopplershift dopplershift force-pushed the fix-appveyor branch 3 times, most recently from 0800c3f to 98e1854 Compare November 16, 2017 20:33
@dopplershift
Copy link
Contributor Author

@Kojoley I just finally understood your comment. Yeah, I think that's what's going on. Let's see if the next one fails now that I took that out.

@dopplershift
Copy link
Contributor Author

Ok, that finally took care of it. Now to fix the stupid repr problems on Python 2.7...

Kojoley
Kojoley approved these changes Nov 17, 2017
View reviewed changes
Copy link
Member
@Kojoley Kojoley left a comment

Choose a reason for hiding this comment

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

LGTM

@dopplershift
Copy link
Contributor Author

So it looks like the lesson to be learned here is: Don't set errorlevel. Ever.

@Kojoley
Copy link
Member
Kojoley commented Nov 17, 2017

Or unset.

Kojoley
Kojoley requested changes Nov 17, 2017
View reviewed changes
Copy link
Member
@Kojoley Kojoley left a comment

Choose a reason for hiding this comment

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

Oops.

Error: No coverage report found

@dopplershift
Copy link
Contributor Author

Looks like that's because we do a git clean...

@Kojoley
Copy link
Member
Kojoley commented Nov 17, 2017

Actually idk why we are making a conda build here. It takes about 30% of time and our conda recipe demands some strange versions of packages (like freetype 3.8, which is available only from conda-forge) and requires to pin anaconda-client to 1.6.3.

@dopplershift
Copy link
Contributor Author

For anaconda-client, we actually 8 lines later say:

conda install --yes conda-build jinja2 anaconda-client

So the pinning doesn't do any good. 🙄

So what we're doing is validating an internal conda recipe for building the package--which is a packaging issue, not a matplotlib code one. We're in control of conda-forge/matplotlib-feedstock, so I don't see why we should be maintaining a separate copy here. IMO, we should probably just defer to conda-forge on that and nuke it from our repo.

@dopplershift
Copy link
Contributor Author

Other things I now see to clean up, which I'll do once we figure out what we're doing with the conda recipe:

  • We configure conda with always_yes set to true, yet we pass --yes everywhere
  • We don't use the CONDA_PY variable
  • We don't use the CONDA_NPY variable--was the idea to test against different numpy versions? Because all we're doing is building conda packages against different versions (and we don't run unit tests on those)
  • TEST_ALL is set to "no" for all builds.
  • TARGET_ARCH is always "x64", and duplicates what's in the platform setting
  • There's a command to fix vcvars64.bat for Python 3.4...which we no longer test.

@Kojoley
Copy link
Member
Kojoley commented Nov 20, 2017

So the pinning doesn't do any good.

#9319 (comment)

@dopplershift dopplershift force-pushed the fix-appveyor branch 2 times, most recently from 41661cb to 49577c6 Compare November 22, 2017 21:22
@dopplershift
Copy link
Contributor Author
dopplershift commented Nov 22, 2017 •
edited
Loading

So I also remove installing obvious-ci--it doesn't seem to be needed to get us to build on AppVeyor. This simplified the conda setup significantly, and eliminated the need to worry about install order and other things. I expect this to work now.

This shaves about 20 minutes off the AppVeyor runs, so that's good.

QuLogic
QuLogic approved these changes Nov 23, 2017
View reviewed changes
Copy link
Member
@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

I can't comment on it directly, but I guess the note about NumPy 1.8 + Python 2.7 can be removed.

@dopplershift
MNT: Remove conda recipe
5edaadb 
Just defer to the conda-forge recipe, since that's what's really being
used. Also stop building conda packages on AppVeyor to save some build
time.
@dopplershift
MNT: Remove AppVeyor workaround for Python 3.4
16512ee 
We're not using here any more.
@dopplershift
Copy link
Contributor Author

I went ahead and remove the outdated comments.

@tacaswell
Copy link
Member

Looks like it uploading to codecov now, merging.

@tacaswell tacaswell merged commit c91c8ad into matplotlib:master Nov 23, 2017
@lumberbot-app
Copy link
lumberbot-app bot commented Nov 23, 2017

There seem to be a conflict, please backport manually

@dopplershift dopplershift deleted the fix-appveyor branch November 24, 2017 18:16
@dopplershift dopplershift mentioned this pull request Nov 24, 2017
Closed
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Nov 24, 2017
@tacaswell
Merge pull request matplotlib#9773 from dopplershift/fix-appveyor
88a20b7 
CI: Make sure AppVeyor fails if tests fail
tacaswell added a commit that referenced this pull request Nov 25, 2017
@tacaswell
Merge pull request #9850 from tacaswell/backport_appveyor_fix
032b374 
Merge pull request #9773 from dopplershift/fix-appveyor
@dstansby dstansby mentioned this pull request Nov 25, 2017
Closed
@anntzer anntzer mentioned this pull request Mar 27, 2018
Merged
6 tasks
@anntzer anntzer mentioned this pull request Jul 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kojoley Kojoley Kojoley requested changes

@QuLogic QuLogic QuLogic approved these changes

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: testing
Projects
None yet
Milestone
v2.1.1
Development

Successfully merging this pull request may close these issues.
5 participants
@dopplershift @dstansby @Kojoley @tacaswell @QuLogic
0