Discussion - make tests against Sphinx dev optional

[trallard]

As noted in #2185 (comment) some recent changes to Sphinx linkcheck have led to our CI/tests to fail against the dev version.
Historically we have used this to catch issues with new/upcoming versions of Sphinx as early as possible but this is right now making all of our CI runs to fail (and we need to by pass the merge rules as the CI workflow is required for merge).

Would it be ok if I took the tests against dev out of our required matrix and make it "optional" for now to have a functional CI again? @drammock @gabalafou

[drammock]

I prefer adding to the tests something that catches the specific new upstream-caused failure (i.e., a new pytest.raises or xfail, a new warning filter in conftest, etc). I prefer this because we don't know how long it will take for this upstream problem to get resolved, and if we're in the habit of ignoring that failure, we might not promptly notice new additional failures due to upstream changes.

That said, I am OK with what you propose on the grounds that it's less work and I don't personally have time to do it the harder way that I'm proposing.

[drammock]

Let's do it the easy way now (make it not required) and then revert that later if and when we find time to do it the hard way.

[gabalafou]

Now that I have dug into the tests quite a bit today, I agree with @drammock that the standard procedure for this should be to create a PR that adds the warning to our known warnings. All it requires is copying the warning message from the logs, pasting it to the intermittent_warnings_list.txt file, committing and pushing to GitHub.

That said, I found another fix that should work just as well:

• Fix Sphinx dev error linkcheck_allowed_redirects #2205

@drammock, on a related topic, looking at warnings_list.txt and intermittent_warnings_list.txt, is it really necessary for us to have two separate files? I understand why they are separate. I understand that if the warnings in warnings_list.txt are NOT encountered at build time, then the build fails. But is that really necessary? Wouldn't it be safe enough to treat all warnings as intermittent warnings — as in, this is a list of warnings that we expect but are not required. The absence of a warning shouldn't be a concern, should it?

[trallard]

I do agree with @drammock that the default should be a more robust approach too (I prefer his original suggestion I prefer adding to the tests something that catches the specific new upstream-caused failure (i.e., a new pytest.raises or xfail, a new warning filter in conftest, etc). better than relying on adding stuff to warnings, but when time is a scarce resource, sometimes a quick fix needs to do.

That said it seems that this is no longer an issue per #2204 but will leave this discussion open for a bit in case we spot a regression

[drammock]

Wouldn't it be safe enough to treat all warnings as intermittent warnings — as in, this is a list of warnings that we expect but are not required. The absence of a warning shouldn't be a concern, should it?

the main advantage I see is that if and when upstream things change such that we're not seeing a particular warning anymore, it forces us to simplify the known warnings list (which is a quick and easy fix), which in turn prompts us to look at the upstream change to see if it merits any other changes to our code besides removing the warning expectation.

Put another way, the (only?) risk to doing it your way is an ever-ballooning list of "maybe expected maybe not" warnings.

[gabalafou]

Oh well I also just now realized that I scrambled things up a bit in my last comment. I realize now that @drammock did NOT propose adding a line to the warnings list. That would have no effect because the warnings list is for the docs CI job whereas the failures related to Sphinx latest/dev were in the test job.

I think I just read "a new warning filter" and immediately concluded it had to do with the warning list because that's what I'm most familiar with.