Instead of commenting out the code, can you remove it entirely? Commented out code like this (especially) is likely to cause confusion on whether it was meant to be reintroduced or removed, and so if we want it gone it should be fully gone (we have git for a reason, if we do ever want it back).

Additionally while we seem to have not triggered any existing tests, tests of the expected behavior would help to ensure we don't break it in the future.

For testing, do you think something like this is enough:

def test_subfigure_zorder():
    # test that we can change the order of a subfigure...
    fig = plt.figure(layout='constrained')
    subfig = fig.subfigures(1, 1)
    subfig.set_zorder(5)
    assert  subfig.get_zorder() == 5

Or should I go up to an image comparison, with something like this, to test that subfigures are indeed plotted under fig level artists if their order is not changed ?

My underlying question is probably "Is there something like too much testing ?" ?

Thanks !

This may require an image test.

The test you wrote will pass on the current release (the problem is that we are ignoring the zorder not that the set_zorder / get_zorder machinery was broken)!

The test needs to check that things are really rendered in the right order which requires looking at the output.

Oh, indeed. I must've been a bit tired when writing this.

Is it better to add a dedicated test, or can I modify an existing test to avoid adding another baseline_image to the folder?

I guess this falls into the general behaviour of subfigures, so I could modify the test test_figure.py:test_subfigure.

For example by adding a legend, and modifying the order of one of the subfigures to verify that one is plotted below, and one on top?

np.random.seed(19680801)
fig = plt.figure(layout='constrained')
sub = fig.subfigures(1, 2)

axs = sub[0].subplots(2, 2)
for ax in axs.flat:
    pc = ax.pcolormesh(np.random.randn(30, 30), vmin=-2, vmax=2)
sub[0].colorbar(pc, ax=axs)
sub[0].suptitle('Left Side')
sub[0].set_facecolor('white')

axs = sub[1].subplots(1, 3)
for ax in axs.flat:
    pc = ax.pcolormesh(np.random.randn(30, 30), vmin=-2, vmax=2)
sub[1].colorbar(pc, ax=axs, location='bottom')
sub[1].suptitle('Right Side')
sub[1].set_facecolor('white')

# below is new
leg = fig.legend(handles=[plt.Line2D([0], [0], label='Line{}'.format(i)) for i in range(5)], loc='center')  # <-- this is new
sub[1].set_zorder(leg.get_zorder() + 1) # <-- this is new

fig.suptitle('Figure suptitle', fontsize='xx-large')

I am not sure if I am supposed to say so, but this is ready for review :) Thanks !

There seem to be some unrelated style changes here, which makes it difficult to look at what exactly you've changed.

Oh, I did not remove the auto-formatting on this project. I will fix that ASAP,

I corrected all commits removing any style change !

Now it is failing check codecov/projetcs/tests, but I remember someone telling me on the Gitter/incubator not to worry too much about this. Is that OK ?

Codecov failing on the tests module is one of the ones we worry about the most (it is easy to write tests that "pass" because they do not run like you expect 😉 ).

Oh, never mind then. Could you give me some pointers on what to do next?

From what I understand, the test is failing due to a decrease in code coverage of the tests.
However, I do not understand how my changes could affect the tests test_tickers.py and test_backends_interactive.py.

From what I understand, this fails because some parts of the tests are not tested due to try.. except.. statements. This looks unrelated to the changes I have made. Should I rebase on main?

This is all really blurry in my head, so please do not hesitate to point out where I am mistaken :)

https://app.codecov.io/gh/matplotlib/matplotlib/commit/265e49ebe3bd3a10979b55b54101eda5676506b6/indirect-changes is from a different PR and shows only one CI job hitting that code (and randomly picking up some coverage)

I am guessing there is only one CI job that actually has the DE locale available and somehow the coverage of code run in the subproccesses is flaky (?!). I opened #27581 as try and make that test run on more of our CI jobs to reduce the odds it gets lost.

Thanks @cgadal! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.