-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[tests] add public properties SphinxTestApp.status
and SphinxTestApp.warning
#12089
Conversation
I wanted to have two implementation of two plugins side-by-side and use the new plugin only for some tests but it's not possible in pytest... So I'll need to merge both the new and legacy implementation together and maintain them so that we can incrementally change the tests. |
Heya, can you explain what this means, as obviously its quite vague:
|
There are many many side-effects which makes it impossible to use with
Each test should be isolated in a way that it does not affect other tests. Some tests use the same test-root but they sometimes change the sources in-place. Other tests import modules but sometimes they do not unload them and thus annotations are kept (and mess other tests). We should never have side-effects like that in general. Also many tests are actually C/C of others and some of them use This PR is actually just one thing I could extract from the huge PR with all the modifications. Also, there are various typing issues that were never resolved or assumptions that were wrong (like, assuming that |
Completely agree, this should be a goal of the test suite 👍 |
Ah yes, actually it's only some parts of the test suite that need to be modernized (like, using monkeypatch instead of manual patching). |
This comment was marked as outdated.
This comment was marked as outdated.
Here's my thought:
def __init__(self, buildername='html', srcdir=None, builddir=..., ...) and inside the constructor we have app = SphinxTestApp()
app = SphinxTestApp('html')
app = SphinxTestApp('html', builddir=...)
app = SphinxTestApp(builddir=...)
srcdir = None
app = SphinxTestApp('html', srcdir) In addition, we have kwargs['srcdir'] = srcdir = sphinx_test_tempdir / kwargs.get('srcdir', testroot) In other words, there will ALWAYS be a Finally, the
In other words, the SphinxTestApp class is NOT meant to be part of the public API. As such, you should only be able to construct a SphinxTestApp instance using Finally, the sphinx marker never included the Note that in Sphinx 4.2, we added the builddir argument but just after the srcdir one. So, we already changed the signature at that time ! and any codebase using positional arguments would likely have been updated since then. I think there shouldn't be a lot of codebase which does not use keyword arguments for constructing a SphinxTestApp unless they really want to rely on a fragile signature. As such, I will still do this kind of breaking changes because 1) the SphinxTestApp class was never meant to be public 2) SphinxTestApp is not meant to be public and only constructed via |
Since the plugin will be a bit long to implement, I'll use separate branch. That way, I can rebase and you can see the diffs of the 3 PRs instead of having every chunk at each PR. EDIT: I cannot transfer the branches easily :( let's forget about this then... |
@chrisjsewell Now it should be fine. I'll change the way the markers (in the next PR) are processed however because the way it was done until now assumed keyword arguments and not just positional ones (except for the 'buildername'). |
Well they are not internals then, they are part of the public API 😉 but yeh thanks @picnixz, I don't wanna block you from moving things forward, and obviously its annoying when things have maybe not have been handled correctly in the past (one of my loves of Rust is that everything is private and immutable by default, so its more diffucult to make these mistakes). But we should be very careful if breaking the "public API"; that it is really needed and, if so, then it is clearly documented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cheers!
I would commit it with a title like "Add SphinxTestApp.status
and SphinxTestApp.warning
public properties" though,
since the current PR title/description is not very explanatory 😅
I'm still not sure about this. I'm OK with the changes to make the But I don't understand the reasoning for making methods public on |
Checking the output, in particular of warnings, is highly common in sphinx extension test suites. You just have to look at https://github.com/search?q=app._warning&type=code, to see the many test suites currently having to use the private attribute for this |
Ok, thank you. People do tend to find and copy code in test cases into non-test code, so I worry that making the attributes public on |
It's something that we must somewhat expose because there is no other way if you don't use the app fixture + status/warning fixture. If you use make_app, you need to access the status/warnings like that. |
I think my concerns are unfounded, and so please go ahead. Thank you both for explaining. So that it's communicated, I'll attempt to explain what I was worrying about:
Why I think my concerns are unfounded:
...and also I can't think of a suggestion to type-check an attribute value on an object that wouldn't require introducing other dependencies or doing unnecessarily complicated things, and I don't think we should do those. So, longwinded explanation, but: I withdraw my concerns. |
No worries! I'll merge tomorrow (from my computer rather than my phone). |
@chrisjsewell Fun fact, I forgot about the commit message ! I just changed the PR title since I don't want to amend commits on master |
SphinxTestApp.status
and SphinxTestApp.warning
@chrisjsewell @picnixz some trivia from archaeological code curiosity: the way that test code used to be provided with the
That approach was deprecated during the migration to |
The legacy of this remains in the current code, in a sphinx/tests/test_builders/test_build_text.py Lines 9 to 15 in bf0bec3
|
It's still being used when using the shared result by the way. You can see it in the |
The way that the parameters were added as (optional, I think?) test arguments is what I think was particularly nice (although they were still Line 82 in 5dd51a7
|
Actually, that's what I would have expected as well because most of the time you do |
personally I feel that having separate fixtures is over-complicating things 😬 but why we are on the subject, actually one of the most annoying things is having to deal with colored output of warnings; |
Mmh, you should technically be able to turn off colors by adding EDIT: Did not work, the logger appears to ignore this. |
I would encorage you to have a look at #12130 first, lets have a think about this from a top-level perspective 😄 |
Well, I agree that I would prefer using
I've looked at it but to me it's something unrelated. Actually, the main issue is that the |
As I've just commented in #12134 (comment), it does actually already obey the colorization setting |
This is a first step to the test suite modernization. I can extract those changes from my current implementation without changing anything (which is good from a review PoV).
cc @jayaddison
btw, the name of the branch is totally unrelated (at first, I just wanted to type things but then I saw that to type them I need this small change).