Improve test engine error feedback:

Improve test engine error feedback:
1. Don't list files that are different just because of comments
2. Show the line numbers from the expected file where the differences are.
3. Allow *-expected.txt files to be marked as skipped -- if first line of file begins with #<skip
4. Add anchor tests which exercise the test engine and #<skip
5. Add a readme.txt for future developers looking to add tests

BUG=120934
TEST=anchor tests exercise the test engine changes


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131003

[aaronlevbugs, 2012-04-02 20:32:52]

Ready for review

[David Tseng, 2012-04-02 23:29:37]

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:61: while
(i < actual_lines_count || j < expected_lines_count) {
Shouldn't this be i < actual_lines_count && j < expected_lines_count? We never
want to access indecies beyond these. You can print the remaining j diff if
necessary after the loop?

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:65: // Skip
comment lines and blank lines in expected output
nit: end the sentence with a period.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:73: if
(!already_inside_diff_block)  // Avoid redundant error reporting
How is this redundant? This would only pick up the first line of a difference
block.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:77: else
Please use the following conventions for multi-lined if/else clauses:
if (...) {
...
} else {
...
}

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:138: if
(!expected_contents.compare(0, 6, "#<skip")) {
Define this string literal as a constant along with the convention of the rest
of the file above.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:170: for
(int i = 0; i < static_cast<int>(diff_lines.size()); i++)
nit: ++i

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:171:
printf(" %d", diff_lines[i]);
Would it be useful to print the expected lines that failed to match and their
line numbers (rather than the entire expected file above and the line numbers
here).

[dmazzoni, 2012-04-02 23:43:59]

I like the diff code except for one case: if the expected file is missing
entirely, I'd like it to print the whole "actual" file as-is, with no line
numbers or anything. The reason for that is so I can write a .html file, send it
to the try server, and copy-and-paste the results directly into the expected
file.

Thanks.

[aaronlevbugs, 2012-04-03 02:30:50]

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:61: while
(i < actual_lines_count || j < expected_lines_count) {
On 2012/04/02 23:29:38, David Tseng wrote:
> Shouldn't this be i < actual_lines_count && j < expected_lines_count? We never
> want to access indecies beyond these. You can print the remaining j diff if
> necessary after the loop?
It was as odd way of ensuring same size files. New patch doesn't have or need
that.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:65: // Skip
comment lines and blank lines in expected output
On 2012/04/02 23:29:38, David Tseng wrote:
> nit: end the sentence with a period.

Done.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:73: if
(!already_inside_diff_block)  // Avoid redundant error reporting
On 2012/04/02 23:29:38, David Tseng wrote:
> How is this redundant? This would only pick up the first line of a difference
> block.
That's the idea, so that if one file has an extra line we don't report every
line as an error until the end of the file.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:73: if
(!already_inside_diff_block)  // Avoid redundant error reporting
On 2012/04/02 23:29:38, David Tseng wrote:
> How is this redundant? This would only pick up the first line of a difference
> block.
New patch doesn't need this.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:77: else
On 2012/04/02 23:29:38, David Tseng wrote:
> Please use the following conventions for multi-lined if/else clauses:
> if (...) {
> ...
> } else {
> ...
> }

Done.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:138: if
(!expected_contents.compare(0, 6, "#<skip")) {
On 2012/04/02 23:29:38, David Tseng wrote:
> Define this string literal as a constant along with the convention of the rest
> of the file above.

Done.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:170: for
(int i = 0; i < static_cast<int>(diff_lines.size()); i++)
On 2012/04/02 23:29:38, David Tseng wrote:
> nit: ++i

Done.

http://codereview.chromium.org/9950068/diff/1/content/browser/accessibility/d...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:171:
printf(" %d", diff_lines[i]);
On 2012/04/02 23:29:38, David Tseng wrote:
> Would it be useful to print the expected lines that failed to match and their
> line numbers (rather than the entire expected file above and the line numbers
> here).
New patch shows all expected lines and marks those which did not match actual
output with a *.

[David Tseng, 2012-04-03 17:15:22]

lgtm

http://codereview.chromium.org/9950068/diff/5001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9950068/diff/5001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:185:
printf("%s","\n");
nit: why not printf("\n")

[aaronlevbugs, 2012-04-03 22:19:20]

http://codereview.chromium.org/9950068/diff/5001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9950068/diff/5001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:185:
printf("%s","\n");
On 2012/04/03 17:15:22, David Tseng wrote:
> nit: why not printf("\n")

Done.

[commit-bot: I haz the power, 2012-04-03 22:24:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aaronlevbugs@gmail.com/9950068/7002

[commit-bot: I haz the power, 2012-04-03 23:13:27]

Try job failure for 9950068-7002 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[dmazzoni, 2012-04-04 05:26:48]

The compile failures are clearly unrelated to this patch - trying again.

On Tue, Apr 3, 2012 at 4:13 PM, <commit-bot@chromium.org> wrote:

> Try job failure for 9950068-7002 (retry) on mac_rel for step "compile"
> (clobber
> build).
> It's a second try, previously, step "compile" failed.
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=mac_rel&**number=15390<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=15390>
>
>
>
http://codereview.chromium.**org/9950068/<http://codereview.chromium.org/9950...
>

[commit-bot: I haz the power, 2012-04-04 05:26:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aaronlevbugs@gmail.com/9950068/7002

[commit-bot: I haz the power, 2012-04-04 06:44:06]

Try job failure for 9950068-7002 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[dmazzoni, 2012-04-04 06:48:28]

These look like real compile errors.

On Tue, Apr 3, 2012 at 11:44 PM, <commit-bot@chromium.org> wrote:

> Try job failure for 9950068-7002 (retry) on mac_rel for step "compile"
> (clobber
> build).
> It's a second try, previously, step "compile" failed.
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=mac_rel&**number=15556<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=15556>
>
>
>
http://codereview.chromium.**org/9950068/<http://codereview.chromium.org/9950...
>

[David Tseng, 2012-04-04 13:35:53]

Yes, I pinged Aaron offline.

There are also missing Mac expectations.

I'll run this through the try bots once a new patch is up.

Thanks,
David

On 4/3/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> These look like real compile errors.
>
> On Tue, Apr 3, 2012 at 11:44 PM, <commit-bot@chromium.org> wrote:
>
>> Try job failure for 9950068-7002 (retry) on mac_rel for step "compile"
>> (clobber
>> build).
>> It's a second try, previously, step "compile" failed.
>> http://build.chromium.org/p/**tryserver.chromium/**
>>
buildstatus?builder=mac_rel&**number=15556<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=15556>
>>
>>
>>
http://codereview.chromium.**org/9950068/<http://codereview.chromium.org/9950...
>>
>

[aaronlevbugs, 2012-04-04 15:45:45]

Ok, boolean->bool -- mental residue from work in another language, and it
compiled on Windows :/
As far as the expectations files, I was hoping I could leave them out and they
would autogenerate, but I can see not.

[aaronlevbugs, 2012-04-04 15:56:15]

On 2012/04/04 15:45:45, aaronlevbugs wrote:
Don't we want the autogenerate expectations feature?

[dmazzoni, 2012-04-04 16:00:14]

I think we don't want to autogenerate expectations on the trybots /
buildbots. In particular, I'm worried about this situation:

1. Someone checks in test.html without test-expected-mac.txt
2. Buildbot autogenerates test-expected-mac.txt
3. Later, someone checks in test-expected-mac.txt
4. Buildbot is unable to patch in that change because a "tracked"
change (from svn) conflicts with an "untracked" locally-added file.

Because of this, we shouldn't autogenerate expectation files. It's
okay to autogenerate "actual" files, and let the user manually rename
them locally.

- Dominic

[dmazzoni, 2012-04-04 16:03:09]

Oh wait - I don't see the new patch. Did you upload?

On Wed, Apr 4, 2012 at 8:45 AM,  <aaronlevbugs@gmail.com> wrote:
> Ok, boolean->bool -- mental residue from work in another language, and it
> compiled on Windows :/
> As far as the expectations files, I was hoping I could leave them out and
> they
> would autogenerate, but I can see not.
>
> http://codereview.chromium.org/9950068/

[aaronlevbugs, 2012-04-04 16:24:02]

New patch up.
boolean->bool
Fake mac expected fiels up for now.

[dmazzoni, 2012-04-04 16:25:59]

The workflow I was proposing was to upload the change, run the try
job, then look at the log of running the test on Mac (or whatever
platform you don't have), and then fix up the changelist.

Will that work?

If there's doubt about the mac results, though, what you did is fine.

Either way, I kicked off a new try job.

- Dominic

[David Tseng, 2012-04-04 16:32:56]

Note that the files do get auto generated -- they're placed into a
temporary location. I don't think we should auto generate and then
place files into our test runs automatically. This should be the
responsibility of the test writer(s).

With that said, I can run the jobs locally and send you the auto
generated temp expectation files if you want a faster workflow (avoid
cut/copy/paste, waiting for try bots/us submitting on your behalf).

On 4/4/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> The workflow I was proposing was to upload the change, run the try
> job, then look at the log of running the test on Mac (or whatever
> platform you don't have), and then fix up the changelist.
>
> Will that work?
>
> If there's doubt about the mac results, though, what you did is fine.
>
> Either way, I kicked off a new try job.
>
> - Dominic
>

[David Tseng, 2012-04-04 16:34:39]

We should also note that those expectations (for mac in this instance)
have not been validated for correctness and are simply baselines.

On 4/4/12, David Tseng <dtseng@chromium.org> wrote:
> Note that the files do get auto generated -- they're placed into a
> temporary location. I don't think we should auto generate and then
> place files into our test runs automatically. This should be the
> responsibility of the test writer(s).
>
> With that said, I can run the jobs locally and send you the auto
> generated temp expectation files if you want a faster workflow (avoid
> cut/copy/paste, waiting for try bots/us submitting on your behalf).
>
> On 4/4/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
>> The workflow I was proposing was to upload the change, run the try
>> job, then look at the log of running the test on Mac (or whatever
>> platform you don't have), and then fix up the changelist.
>>
>> Will that work?
>>
>> If there's doubt about the mac results, though, what you did is fine.
>>
>> Either way, I kicked off a new try job.
>>
>> - Dominic
>>
>

[dmazzoni, 2012-04-04 16:51:52]

Yes - if you don't want to validate the Mac roles/states against the
accessibility guide right now, I prefer checking in "skip" files.

Either way is fine, it's good to have both options.

- Dominic

On Wed, Apr 4, 2012 at 9:34 AM, David Tseng <dtseng@chromium.org> wrote:
> We should also note that those expectations (for mac in this instance)
> have not been validated for correctness and are simply baselines.
>
> On 4/4/12, David Tseng <dtseng@chromium.org> wrote:
>> Note that the files do get auto generated -- they're placed into a
>> temporary location. I don't think we should auto generate and then
>> place files into our test runs automatically. This should be the
>> responsibility of the test writer(s).
>>
>> With that said, I can run the jobs locally and send you the auto
>> generated temp expectation files if you want a faster workflow (avoid
>> cut/copy/paste, waiting for try bots/us submitting on your behalf).
>>
>> On 4/4/12, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
>>> The workflow I was proposing was to upload the change, run the try
>>> job, then look at the log of running the test on Mac (or whatever
>>> platform you don't have), and then fix up the changelist.
>>>
>>> Will that work?
>>>
>>> If there's doubt about the mac results, though, what you did is fine.
>>>
>>> Either way, I kicked off a new try job.
>>>
>>> - Dominic
>>>
>>

[commit-bot: I haz the power, 2012-04-05 05:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aaronlevbugs@gmail.com/9950068/13001

[commit-bot: I haz the power, 2012-04-05 08:42:27]

Try job failure for 9950068-13001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-05 15:32:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aaronlevbugs@gmail.com/9950068/13001

[dmazzoni, 2012-04-05 15:32:29]

Argh, trying again...

On Thu, Apr 5, 2012 at 1:42 AM, <commit-bot@chromium.org> wrote:

> Try job failure for 9950068-13001 (retry) on win_rel for step
> "browser_tests".
> It's a second try, previously, step "browser_tests" failed.
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=win_rel&**number=18320<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=18320>
>
>
>
http://codereview.chromium.**org/9950068/<http://codereview.chromium.org/9950...
>

[commit-bot: I haz the power, 2012-04-05 20:32:16]

Change committed as 131003