Closed Bug 922581 Opened 11 years ago Closed 7 years ago

Test manifests should expose reason for being inactive

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: gps, Unassigned)

Details

Often when updating test manifests to skip tests or expect non-expected results, we write a comment saying why we did these things. After parsing, this comment doesn't exist because, well, it's a comment. These comments are useful. They often document assertions about the state of something, etc. These things change over time. But people don't go into the manifests verifying everything appears sane. What I'd like to see is the ability for our tools to output a list of all inactive tests *along with* the reason why these tests aren't being executed. That way, it's easy to get a complete summary of why things aren't being executed. This should, in turn, cause people to start asking questions and should hopefully lead to fewer tests being marked as inactive. We can start this process by having the manifest capture reasons why tests are skipped. I'd even go so far as to require a human readable explanation every time a test varies from the expected norm! FWIW, the "run-sequentially" key already works kinda like this - people stuff a reason in the value. e.g. "run-sequentially = Uses a hardcoded http port." How we do this, I don't know. Do we expand the filter grammar to support an open text field? Do we expose new properties to hold strings? I'm not sure. I think the first step is agreeing we want to do this. Then we can figure out an implementation.
Do you mean disabled or run-if? Both options support a description already, similar to the one you gave above: [testFooBar.js] disabled = Bug 12345 - Poor handling of foo bars
skip-if doesn't handle descriptions, does it? If you look at all the .ini files, you'll find many comments. IMO these should all be captured as descriptions somehow.
Do you have an example? In our mozmill tests we already make use of: skip-if = os == 'mac' = Bug 840022 - Test failure 'The forward button has been made visible for the 1 page' It seems to work (see the last skipped test): http://mozmill-staging.blargon7.com/#/functional/report/6ec6776efe900da3fd2b64a7505e6bff > skip-if: os == 'mac' = Bug 840022 - Test failure 'The forward button has been made visible for the 1 page' We might only print the description after the condition.
Oh, I didn't realize that funky syntax works! I don't see it used *at all* for xpcshell.ini manifests in mozilla-central! The only descriptions I see in skip-if in m-c are |skip-if = "FTP channel implementation needs to migrate to the new cache backend API"|. So, I guess that reduces this bug to be about having the manifest parser extract the description as something useful. For all I know, it does that already! Perhaps we should get the sheriffs starting to annotate manifests this way...
For reference, the manifest line in question is here: http://hg.mozilla.org/qa/mozmill-tests/file/4155a6e8a4ab/tests/functional/testToolbar/manifest.ini#l2 I don't think this is an actual feature of the manifest parser or the expression language. I think you've just stumbled upon an edge case that happens to work. AFAICT, the entire string on the right side of the first = gets passed to the expression parser, which uses re.Scanner to tokenize, and re.Scanner returns a list of tokens plus whatever string is leftover that it couldn't tokenize, which the expression parser just throws away: https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L184 Realistically we should probably be throwing an error here because this expression is malformed.
On a somewhat different note, I'd love to see the same filter functionality (skip-if, disabled, etc.) on include directives in addition to what currently exists for individual tests. But this is bait for a different bug.
Mass closing bugs with no activity in 2+ years. If this bug is important to you, please re-open.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.