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

Test plot #2141

Closed
wants to merge 4 commits into from
Closed

Test plot #2141

wants to merge 4 commits into from

Conversation

marqh
Copy link
Member

@marqh marqh commented Sep 14, 2016

Removal of

  • iris.tests.test_plot
  • iris.tests.test_quickplot

these are already widely unit tested

Moved

  • iris.tests.test_mapping
    to
  • iris.tests.integration.test_mapping
    and altered a few tests to use quickplot instead of plot

these are better integration tests, looking at Iris, cartopy and matplotlib together

I assert that this change set makes a small impact to our test coverage, the plot code is still well tested, with wide spread unit testing in place and a sensible set of integration tests.

The example gallery also provides integration level testing of example code, producing plots

@bjlittle
Copy link
Member

@marqh Unfortunately, I'm 👎 on this PR.

I understand your motivation to purge both test_plot.py and test_quickplot.py,

  • their core capability hasn't really changed since the initial commit ~4 years ago
  • yes, we do have unit test coverage in these areas now, and
  • yes, they're both sensitive to matplotlib distribution version changes thanks to check_graphic

However,

  • we really don't know if we've lost test coverage by removing both these "integration" (?) tests
  • the nub of the motivation behind this PR (as I understand it) is to lessen our dependence on check_graphic ... so fundamentally (for me) it's that that we really need to change.

If we can address the check_graphic issue head-on, then I feel we can then re-address purging / re-factoring test_plot.py and test_quickplot.py and other such aging "integration" tests.

I think it would be very valuable for other @SciTools/iris-devs opinion here ...

@marqh
Copy link
Member Author

marqh commented Sep 15, 2016

the nub of the motivation behind this PR (as I understand it) is to lessen our dependence on check_graphic ... so fundamentally (for me) it's that that we really need to change.

I'd like to disagree with this perspective.

The motivation behind this change is to make it easier to lessen our version pinning because of check_graphic.

I believe that all solutions to check_graphic improvements so far involve some degree of image maintenance and analysis, and this make me feel that the more we use such an approach, the more we will struggle to implement a solution without it having significant knock on consequences.

I think that this approach is a key part of the work to

address the check_graphic issue head-on

i think that this is a key enabling factor in allowing work to take place to make check_graphic more flexible whilst still useful.

I fear that without a change of this style then we won't be able to get a good solution to check_graphic, due to the scale of issue it will need to solve.

So, if the nub of the worry is

we really don't know if we've lost test coverage by removing both these "integration" (?) tests

how do we address this?

@bjlittle if this were addressed, would you be content with the approach?

@QuLogic
Copy link
Member

QuLogic commented Oct 7, 2016

we really don't know if we've lost test coverage by removing both these "integration" (?) tests

how do we address this?

Finish #2151, most probably.

@pelson
Copy link
Member

pelson commented Oct 7, 2016

I'm in agreement with @bjlittle here. As it stands, this PR is not mergable. I think we all want to see shorter test run time, and I agree with the approach you have taken in #2139 in reducing the tests on individual merit (although @QuLogic's comment about Anscome's quartet is extremely pertinent).

@pelson pelson closed this Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants