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

Prepare for cross-framework test suite #6920

Merged

Conversation

Kojoley
Copy link
Member

@Kojoley Kojoley commented Aug 7, 2016

I have made a separated PR for this commit because I need a review for it (this is the base for other changes related to pytest framework support).

Changes:

  • introduced xfail(reason) function
    • uses: raise KnownFailureTest(msg) -> xfail(reason)
    • same name and signature as in pytest
  • introduced skip(reason) function
    • uses: raise SkipTest(msg) -> skip(reason)
    • same name and signature as in pytest
  • introduced skipif(condition, reason=None) decorator
    • uses: replaces def func(): if condition: skip()
    • same name and signature as in pytest
      • can be used with functions, classes, and methods
      • supports string condition (evaluated at runtime)
  • moved nose related code to testing.nose submodule
    • plugins in testing.nose.plugins submodule
    • decorators implementation in testing.nose.decorators
      (interface is still in testing.decorators, implementation will
      have been chosen at runtime according to used test framework)
  • matplotlib.test function unifications
  • tests.py now uses matplotlib.test()

@jenshnielsen
Copy link
Member

👍 from my first quick read of the changes

@Kojoley
Copy link
Member Author

Kojoley commented Aug 8, 2016

Small fix: rcParams['backend'] -> get_backend()

try:
import nose
try:
from unittest import mock
Copy link
Member

Choose a reason for hiding this comment

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

mock is used in other tests; not sure this test should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Check is not removed, it is just moved to check_deps function

@Kojoley Kojoley force-pushed the prepare-for-cross-framework-test-suite branch 2 times, most recently from 7dc73af to df0d119 Compare August 10, 2016 00:00
@story645 story645 mentioned this pull request Aug 10, 2016
5 tasks
@Kojoley Kojoley force-pushed the prepare-for-cross-framework-test-suite branch 2 times, most recently from 9d7ef8d to 362c271 Compare August 10, 2016 10:25
@tacaswell tacaswell added this to the 2.1 (next point release) milestone Aug 13, 2016
@tacaswell
Copy link
Member

This needs a rebase

  - introduced `xfail(reason)` function
    - uses: `raise KnownFailureTest(msg)` -> `xfail(reason)`
    - same name and signature as in pytest

  - introduced `skip(reason)` function
    - uses: `raise SkipTest(msg)` -> `skip(reason)`
    - same name and signature as in pytest

  - introduced `skipif(condition, reason=None)` decorator
    - uses: replaces `def func(): if condition: skip()`
    - same name and signature as in pytest
    - can be used with functions, classes, and methods
    - supports string condition (evaluated at runtime)

  - moved nose related code to `testing.nose` submodule
    - plugins in `testing.nose.plugins` submodule
    - decorators implementation in `testing.nose.decorators`
      (interface is still in `testing.decorators`, implementation will
       have been chosen at runtime according to used test framework)

  - `matplotlib.test` function unifications
  - `tests.py` now uses `matplotlib.test()`
@Kojoley Kojoley force-pushed the prepare-for-cross-framework-test-suite branch from 362c271 to f20efb9 Compare August 14, 2016 10:02
@Kojoley
Copy link
Member Author

Kojoley commented Aug 16, 2016

Ping

@@ -1,15 +1,3 @@
class KnownFailureTest(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

I would rather not move this for compatibility reasons.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I am pretty sure other projects actually uses our KnownFailure stuff.

On Wed, Aug 17, 2016 at 1:15 PM, Thomas A Caswell notifications@github.com
wrote:

In lib/matplotlib/testing/exceptions.py
#6920 (comment):

@@ -1,15 +1,3 @@
-class KnownFailureTest(Exception):

I would rather not move this for compatibility reasons.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/matplotlib/matplotlib/pull/6920/files/f20efb99d67e581f18f8fd58db07d6279528fcbd#r75165500,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-A6W8iBehFuv_ltVBw91aSSmfrI6ks5qg0GmgaJpZM4Jemgm
.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot agree with this for two reasons:

  • PRs that relay on raise KnownFailureTest should not pass after this PR ever, and this change will force them to switch on xfail
  • Other projects must not relay on matplotlib testing features, it is their problem if they are

I can understand backward compatibility on matplotlib core features, but with removing nose support all this stuff will gone. I have already tried my best to make dual nose-pytest support, but leaving nose-specific stuff on the current places will blow something in any time in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Just to note, matplotlib and many other projects depend on some of numpy's
testing features. By your logic, that shouldn't be done and every project
should implement their own numerical comparison tools?

I am not exactly sure how to proceed here.

On Wed, Aug 17, 2016 at 1:31 PM, Nikita Kniazev notifications@github.com
wrote:

In lib/matplotlib/testing/exceptions.py
#6920 (comment):

@@ -1,15 +1,3 @@
-class KnownFailureTest(Exception):

I cannot agree with this for two reasons:

  • PRs that relay on raise KnownFailureTest should not pass after this
    PR ever, and this change will force them to switch on xfail
  • Other projects must not relay on matplotlib testing features, it is
    their problem if they are

I can understand backward compatibility on matplotlib core features, but
with removing nose support all this stuff will gone. I have already tried
my best to make dual nose-pytest support, but leaving nose-specific stuff
on the current places will blow something in any time in the future.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/matplotlib/matplotlib/pull/6920/files/f20efb99d67e581f18f8fd58db07d6279528fcbd#r75168432,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-HYvL9iMog7FVqaAIeBGMR-v5ev-ks5qg0WIgaJpZM4Jemgm
.

Copy link
Member Author

Choose a reason for hiding this comment

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

By my logic - anything that is not a part of the public API can be changed or even removed at any time, so everyone who did something like using KnownFailureTest makes it on its own risk.

Copy link
Contributor

Choose a reason for hiding this comment

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

Numpy's tools are useful for testing using numpy arrays, so I don't find referencing them to be a compelling argument. The exceptions removed here are not unique to matplotlib functionality. It was a bad idea to for people to use them from us in the first place, and are trivially recreated for anyone down stream who happen to use them.

@tacaswell
Copy link
Member

'powercycled to try and trigger coveralls

@tacaswell
Copy link
Member

Merging this as it is blocking progress for @story645

Independent of if the testing code should be part of the public API, people are using it that way (and to my knowledge) we have not been actively discouraging it. We are going to have to eventually shim these back or document the API changes.

@tacaswell
Copy link
Member

and the travis failure is an install failure on the basemap dependency.

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.

None yet

7 participants