Bug 49465 – Enabling test expectations analyzer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Enabling test expectations analyzer

Created attachment 73766 [details]
Patch

This should be non-intrusive change.

The results of analysis using this file can be found in
http://www.corp.google.com/~imasaki/html/media_all_testcases.html

[Victoria Kirst]

A few comments, but other than those things, LGTM! (I am not a WebKit committer though, so you should get someone else to take a look at this as well.)

> LayoutTests/platform/chromium/test_expectations.txt:56
> +// -KNOWNISSUE, UNIMPLEMENTED and TESTISSUE keyword in comments were added for analysis purpose

I think you might want to delete this line (or at least document it somewhere else) because these keywords aren't part of the "official" syntax for test_expectations.txt.

> LayoutTests/platform/chromium/test_expectations.txt:2924
> +BUGDPRANKE BUG60845 WIN LINUX : media/video-poster.html = PASS TEXT

I'm not sure what BUG<name> means. Does that mean dpranke is the owner of that bug? If so, maybe you should have just BUG60845 and make dpranke the owner of that bug?

Created attachment 74277 [details]
Patch

[David Levin]

Comment on attachment 74277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74277&action=review

> LayoutTests/ChangeLog:3
> +        Reviewed by vrk

Don't fill in this line. Leave it as NOBODY(OOPS).

If you were to fill it in, you'd write out the full name of the person, not part of their email addresss.

> LayoutTests/ChangeLog:5
> +        change

What is this about?

> LayoutTests/ChangeLog:7
> +        Enabling test expectations analyzer

This doesn't make sense to the outside observer (which I consider myself).

There is no enabling of anything in this bug only adding a bunch of BUG* to various lines in test_expectations.

Also it is totally unclear what the objective is and why those BUG* are being added.

Please concisely explain why you are adding this change to test_expectations (mostly adding more BUG* to various lines and removing other BUG*).

> LayoutTests/platform/chromium/test_expectations.txt:2922
> +// BUG60845 : Adding the sleep  may improves the stablility

two spaces after sleep

[David Levin]

(In reply to comment #3)
> The results of analysis using this file can be found in
> http://www.corp.google.com/~imasaki/html/media_all_testcases.html

This isn't really helpful to the public webkit community to understand anything. (fwiw, I can access it and it is a big colorful! table of these test but I'm not sure what is trying to tell me with a brief glance.)

Here's a hint about this bug: I don't think it is about "Enabling test expectations analyzer" at all :) -- since no analyzer is included in the patch or enabled in it (so the title will likely need to change).

Hi, David,

I am sorry about I did not put enough information about this bug is all about. I am working on sorting out test expectation file entries. My goal is to understand and analyze test expectation entries for media related test cases in automatic way. So, I developed scripts called test expectations analyzer. 
Here is the presentation link that I made recently about this analyzer.
https://docs.google.com/a/google.com/present/edit?id=0AbFh834-Y6t5ZGM5Ym0yNm5fMGZ2NTdoY2dy&hl=en

This changes in this bug have the following objectives:
(1) assign test case to bug more specifically (Ideally, one to one for testcase failure to bug rather than one bug associated many test case failures)
(2) add tags (UNIMPLEMNTED, KNOWNISSUE, TESTISSUE) in comments portion of test analyzer to each test case 
(3) add more comments as result of my manual analysis https://spreadsheets0.google.com/a/google.com/ccc?key=t11rK9tFkyxtODpifbNNQFg&hl=en#gid=0 

As for 
http://www.corp.google.com/~imasaki/html/media_all_testcases.html

It shows the test expectation analyzer result. Here test case color according to the following criteria
GREEN: test case that pass
RED: test case that are identified as UNIMPLEMNTED or KNOWNISSUE or TESTISSUSE. In other words, test case should fail all the time. The change for this bug (adding meta data in comments) are needed to make test cases RED.
YELLOW: otherwise

The analyzer code itself is currently being polished for code review now. 

If you have any question, please let me know.

Thank you.
Kenji

[David Levin]

(In reply to comment #8)
> assign test case to bug more specifically (Ideally, one to one for testcase failure to bug rather than one bug associated many test case failures)

This is ideally what what your bug title should be. Here's my attempt (if I understood you correctly): "Assign bugs with test cases to failure chromium's test_expectations.txt"

Feel free to create a new bug with a title like this or something more accurate and if you wish, make this bug (49465) dependent on that new bug.

I hope that makes sense. The goal is to have the patch directly address the bug. Then people can look at the bug and tell what the patch is attempting to do. And this also usually means you ideally have one patch per bug in general.

When you post that patch in that bug, it would hopefully say something about bug numbers that were changed or why there is more than one bug assigned to some layout tests. (I'm guessing it means that there are multiple features needed for that test to succeed.)
 
For example: http/tests/media/remove-while-loading.html (had the original bug number removed but that bug number remains on other tests). (I'm guess it is because bug 13907 was too generic but then why does it remain on other tests which have another bug assigned like this one http/tests/security/local-video-poster-from-remote.htm).

I appreciate the way that you are breaking this up into small steps for patches. I'm just trying to help you to break up the bugs in a similar manner (and try to have the bug be self-contained). This will help others (like reviewers) understand things without getting familiar with everything you are doing. 

I hope this will give you a clear path to getting this done quickly!

Submitted 
http://trac.webkit.org/changeset/72945