Closed Bug 756083 Opened 10 years ago Closed 9 years ago

Allow easier updating of expected failures for imported testharness.js tests

Categories

(Testing :: Mochitest, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: ayg, Assigned: Ms2ger)

References

Details

Attachments

(2 files, 1 obsolete file)

It would be nice if we had a script of some kind that would parse mochitest log output and generate all the appropriate JSON files.  This bash snippet almost does it, for one file:

(echo '{'; grep 'TEST-.*-FAIL.*test_runtest' /tmp/log | sed 's!.*/tests/dom/imptests/editing/conformancetest/test_runtest.html | !!;s/; \(assert\|Component returned\).*//;s/\\/\\\\/g;s/"/\\"/g;s/^/  "/;s/$/":true,/'; echo '}') > failures/editing/conformancetest/test_runtest.html.json

But a) it leaves an extra trailing comma; b) it assumes all assert failures start with either "assert" or "Component returned", which is false; c) it doesn't generate makefiles; d) it's written in bash (instead of, say, Python).  It's usable for now for my purposes, but it would be nice if we had something better.

Fixing (b) will require adding some distinctive marker to distinguish the test name and the assert, ideally something that can't actually occur in the test and/or assert names.
Flags: in-testsuite-
This was broken by bug 763169 part c.  Now not all the known fails are output, so there's no way to reconstruct the list of expected fails from the log.  You'd have to splice it into the existing list somehow.  Doable, although the order will be somewhat random.  (For now I hacked around it by temporarily adjusting testharnessreport.js before running the tests.)
Assignee: nobody → Ms2ger
Comment on attachment 640086 [details] [diff] [review]
Part a: Factor out Makefile utilities

looks good
Attachment #640086 - Flags: review?(jhammel) → review+
Comment on attachment 640087 [details] [diff] [review]
Part b: Introduce code to make it easier to update expected failures for imported testharness.js tests

please use 4-space indenation
Comment on attachment 640087 [details] [diff] [review]
Part b: Introduce code to make it easier to update expected failures for imported testharness.js tests

+* parseFailures.py
+  Parses failures out of a mochitest log and writes out JSON files
and Makefiles
+  into the correct failures/ folder.

Should probably note where the '@@@' magic string comes from.

+import json

This will only work with python 2.6 or later.  I'm not sure what the
minimum we support in m-c is, but if we do support 2.5 or earlier we
should do something different here (fwiw, simplejson is in the tree,
though you'll have to be creative to use it).

+  dir = path[:path.rfind('/')]

use os.path.dirname() instead, unless there's a reason not to

+    dirp, leaf = path.rsplit('/', 1)

could use os.path.split()

+    if dirp not in pathmap:
+      pathmap[dirp] = []
+    pathmap[dirp].append(leaf)

pathmap.setdefault(dirp, []).append(lead)

+    fp = open(k + '/Makefile.in', 'wb')

os.path.join(k, 'Makefile.in')  There are a few other places where you
could use os.path.join too


+if __name__ == '__main__':
+  main(sys.argv[1])

Please don't do this.  If there aren't any arguments you'll err out
with an IndexError.  Either try/except or check the length of
sys.argv to ensure its appropriate.


Giving an r- for now.  Please resubmit with these nits fixed or explained and 4-space indentation as it looks very close
Attachment #640087 - Flags: review?(jhammel) → review+
(In reply to Jeff Hammel [:jhammel] from comment #6)
> +import json
> 
> This will only work with python 2.6 or later.  I'm not sure what the
> minimum we support in m-c is, but if we do support 2.5 or earlier we
> should do something different here (fwiw, simplejson is in the tree,
> though you'll have to be creative to use it).

*shrug*. Aryeh and I are the only people who are going to run this script anyway.
Yeah, this script isn't run as part of building or anything.  It's only ever going to be a couple of people who need to run it, and only occasionally at that, so I think it's fine to write it in whatever.
(In reply to Jeff Hammel [:jhammel] from comment #6)
> +  dir = path[:path.rfind('/')]
> 
> use os.path.dirname() instead, unless there's a reason not to

This isn't an OS path, it's a URL, so I don't think it makes sense to use that here. Same for most of your other comments.

> +    if dirp not in pathmap:
> +      pathmap[dirp] = []
> +    pathmap[dirp].append(leaf)
> 
> pathmap.setdefault(dirp, []).append(lead)

Neat.

> Giving an r- for now.

That's not what you did ;)
Comment on attachment 640561 [details] [diff] [review]
Part b: Introduce code to make it easier to update expected failures for imported testharness.js tests (v2)

looks good
Attachment #640561 - Flags: review?(jhammel) → review+
https://hg.mozilla.org/mozilla-central/rev/afc9d034a247
https://hg.mozilla.org/mozilla-central/rev/dcca069277d0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 794891
You need to log in before you can comment on or make changes to this bug.