On 2015-06-10 3:38 AM, David Rajchenbach-Teller wrote:
However, I do believe in the following scenario: - oh, there is an assertion warning, it's not my fault, let's allow one assertion; - wait, there are a few, let's allow several; - oh, they are intermittent, let's make it an interval; - [at some point, some of the warnings are fixed, but nobody realized];
Yes, assuming the number of assertions doesn't fall outside of the range, of course.
- [at some point, something causes other warnings to be triggered, but nobody realized]; - we realize the regression years later.
Yes, like I said, I agree this problem exists _in theory_. I however think that it's unlikely. (Note that we currently have only 82 SimpleTest.expectAssertions annotations in our tests, so we're already talking about an edge case of something that is extremely uncommon itself. :-)
I am almost sure that I have witnessed this kind of scenario while working on Session Restore. It used to cause assertion warnings at some place in the DOM, which were not documented in any bug that I could find, and nobody cared.
Concrete examples would help. I can't find any test in browser/components/sessionstore right now with an assertion annotation so it's hard to say what the exact issue that you saw was, but I think you are complaining about the lack of documentation and that nobody cared. Neither of those issues will be solved with what you are suggesting.
Also, second issue of the current approach: I have seen very few (if any) bugs yet that documents exactly the assertion warnings that they trigger, explaining why they are normal and should be ignored. That's a codetrap for whoever comes next. Requiring a test to actively opt-in for whitelisting a specific assertion warning means that there is a good place to r- the test until this is documented.
Not necessarily. Reviewers can choose to require exactly that everywhere SimpleTest.expectAssertions is used (I for one try to do that, but these are so rare that I can't remember the last time I reviewed such a test very well...)
My point is, changing |SimpleTest.expectAssertions(2)| to |SimpleTest.expectAssertions(["first assertion message", "second assertion message"])| doesn't really give us much besides the ability to pinpoint the expected assertions to the exact assertions that the test triggers. And I'm not saying that is not valuable, it's just a solution to an extremely rare problem.
Issues such as lack of documentation of why the test is triggering an assertion are orthogonal to the syntax we use to declare the whitelist.
Frankly, I'm really sick of warnings, and anything we can do to make them more actionable would be one step towards restoring sanity in our tests and in some parts of our codebase.
Warnings are different. There is basically a spectrum of things you can do, from setting the MOZ_IGNORE_WARNINGS (and MOZ_QUIET) environment variables if you're just annoyed with them and don't want to see them, to filing bugs for the ones that happen way too often in tests (or during common operations such as opening a tab, typing something in a form, etc) and CCing the relevant people and asking whether the warning can be silenced, etc. I very much welcome the latter and have tried to act on such bugs where I can. Right now we have so many warnings that a project to annotate every test with every single warning that it generates is *really* impractical. And once we fix the problem of having too many frequently occurring one, the value of such a project is heavily diminished.
Cheers, Ehsan _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
