Allow pytest disable warnings in CI #28077
Conversation
dstandish
requested review from
ashb,
jedcunningham,
kaxil and
potiuk
as code owners
|
|
||
| LOW_MEMORY_CONDITION = 8 * 1024 * 1024 * 1024 | ||
|
|
||
| DISABLE_WARNINGS_LABEL = "disable warnings" |
There was a problem hiding this comment.
I believ we wanted to "enable warnings" by label so that they are hidden by default ?
There was a problem hiding this comment.
even better --- i thought you wanted them there by default
There was a problem hiding this comment.
Not really. I think it was more of @uranusjr's quest to fix them and I think he turned them on in the first place, for that reason, but I think indeed this should be more ore less dedicated effort to remove them and showing them by default when we have many of them do not play well with GitHub UI.
There was a problem hiding this comment.
yeah, i agree. i also appreciate the desire to squash those warnings. there's a lot of noise... and if it shows up in the tests, probably a lot of it shows up for users.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
One nit, one doubt.
I like it this way - i wanted to do it via selective-checks, but this way is indeed simpler and easy to add other test variations this way.
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
Let's not? Let's remove/fix the warnings instead?
|
@ashb - Absolutely. I'd'love that. What do you propose exactly ? How do you think we can improve curent situation? Currently the warnings are printed and besides slowing the ways how people can se their test failures, they don't do much - that's the nature of warnings. It's lile that for quite some time. I have my own idea but it's quite disruptive and might be difficult to accept so I would love to hear, maybe others have better ideas before I bias them with mine. |
|
My idea is a Two phase approach, similar to how we did... Typing? in the past. And I call it a ratchet. The first step is to capture how many warnings each test file produces, and then we fail ci if any new warnings are issued. The next step is once a file doesn't have any warnings there is a way of making pytest treat warnings as failure (I think we do it in one place in our test site right now). I noticed the sheer number of warnings when looking at the bug fix for rc3: it's way up from where I remember it, and it was only my list to look at next week. I suspect over half are from one of two helper funcs in our test suite! |
|
Yes. This is good idea :). |
|
Closing as we have better solution in #28096 . But .. follow up on actually fixing the warnings is a good idea :) |
3 participants
No description provided.