Don't require expectations file for DumpAccessibility tests.

Don't require expectations file for DumpAccessibility tests.

Instead of requiring an expectations file for DumpAccessibilityTree tests,
just skip tests without an expectations file on that platform. Make it easier
to rebaseline expectations by automatically overwriting expectations of
failing tests when the user passes a command-line switch to
content_browsertests: --generate-accessibility-test-expectations

This lets us delete a bunch of empty expectations files and more easily
see at a glance which files are missing expectations for which tests.

BUG=none

Committed: https://crrev.com/fb8139fb49877ee41f97f6dc9bc29a5b6e52b5a6
Cr-Commit-Position: refs/heads/master@{#364455}

[dmazzoni, 2015-12-08 19:26:00]

dmazzoni@chromium.org changed reviewers:
+ creis@chromium.org, nektar@chromium.org

[dmazzoni, 2015-12-08 19:26:01]

nektar: please review
creis: please approve changes to content_switches

[Charlie Reis, 2015-12-08 22:52:05]

content_switches LGTM.

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:137:
LOG(INFO) << "Testing: " << file_path.LossyDisplayName();
We're generally supposed to use DVLOG, right?
http://www.chromium.org/developers/coding-style#TOC-Logging

I don't feel strongly, though, since it's a test framework and this is intended
to be informational.

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:152: << "
(it can be blank) and then run content_browsertests "
s/blank/empty/

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:250:
LOG(ERROR) << "Wrote expectations to: "
Why ERROR in this case?

[dmazzoni, 2015-12-08 23:22:42]

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:137:
LOG(INFO) << "Testing: " << file_path.LossyDisplayName();
On 2015/12/08 22:52:05, Charlie Reis wrote:
> We're generally supposed to use DVLOG, right?
> http://www.chromium.org/developers/coding-style#TOC-Logging
> 
> I don't feel strongly, though, since it's a test framework and this is
intended
> to be informational.

Yeah, it makes it harder to figure out what happened if the test failed, without
this. This is especially important here because these tests have different
expectations on each platform so we often run a try job and look for failures on
bots, not locally - so DVLOG isn't helpful.

[nektarios, 2015-12-09 00:04:59]

dump_accessibility_browsertest_base.cc
      EXPECT_TRUE(base::WriteFile(
          expected_file, actual_contents.c_str(), actual_contents.size()));

Since this is not a test expectation, isn't it better if we check with an "if"
statement and if it fails wlog a different message indicating failure?

Secondly, I noticed that you replaced some printf function calls with LOGs.
Could you do it for all the instances in this file?

build.gn
I noticed that the code is not compiled on Linux. But don't we have auralinux
tests that might need it.

[nektarios, 2015-12-09 00:15:48]

On 2015/12/09 at 00:04:59, nektarios wrote:
> dump_accessibility_browsertest_base.cc
>       EXPECT_TRUE(base::WriteFile(
>           expected_file, actual_contents.c_str(), actual_contents.size()));
> 
> Since this is not a test expectation, isn't it better if we check with an "if"
statement and if it fails wlog a different message indicating failure?
> 
> Secondly, I noticed that you replaced some printf function calls with LOGs.
Could you do it for all the instances in this file?
> 
> build.gn
> I noticed that the code is not compiled on Linux. But don't we have auralinux
tests that might need it.

Ignore my last comment, the code *is* compiled on Linux.

[nektarios, 2015-12-09 00:16:01]

lgtm

[nektarios, 2015-12-09 01:12:35]

Please update the readme file under test/data/accessibility to mention this
change.
While you are at it, you might want to add a comment about the types, the format
and the priority of @ directives. E.g. @ALLOW or @DENY directives that come
later in the test file overwrite the effect of previous ones. I spent a few
minutes figuring this out from the code and having it in the readme file would
have made my job easier.

[dmazzoni, 2015-12-09 19:38:03]

On 2015/12/09 00:04:59, nektarios wrote:
> dump_accessibility_browsertest_base.cc
>       EXPECT_TRUE(base::WriteFile(
>           expected_file, actual_contents.c_str(), actual_contents.size()));
> 
> Since this is not a test expectation, isn't it better if we check with an "if"
> statement and if it fails wlog a different message indicating failure?

Sure, I can see how using EXPECT makes it seem like a test expectation. How
about CHECK instead? If that fails there will be enough information logged to
figure out what file couldn't be written. It should be rare.

> Secondly, I noticed that you replaced some printf function calls with LOGs.
> Could you do it for all the instances in this file?

Sure, done. The main thing I wanted was for the diff to not have a prefix.

[dmazzoni, 2015-12-09 20:17:21]

On 2015/12/09 01:12:35, nektarios wrote:
> Please update the readme file under test/data/accessibility to mention this
> change.

Great idea.

> While you are at it, you might want to add a comment about the types, the
format
> and the priority of @ directives. E.g. @ALLOW or @DENY directives that come
> later in the test file overwrite the effect of previous ones. I spent a few
> minutes figuring this out from the code and having it in the readme file would
> have made my job easier.

Done. I filled in information about the events tests too, and some of the more
advanced features that are only used in a few tests currently. You might learn
something!

[dmazzoni, 2015-12-09 20:24:07]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-09 20:24:08]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1505903002/#ps100001
(title: "Fix compile error")

[commit-bot: I haz the power, 2015-12-09 20:27:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505903002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505903002/100001

[commit-bot: I haz the power, 2015-12-09 21:40:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 21:40:27]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2015-12-09 22:23:19]

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:152: << "
(it can be blank) and then run content_browsertests "
On 2015/12/08 22:52:05, Charlie Reis wrote:
> s/blank/empty/

Done.

https://codereview.chromium.org/1505903002/diff/60001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:250:
LOG(ERROR) << "Wrote expectations to: "
On 2015/12/08 22:52:05, Charlie Reis wrote:
> Why ERROR in this case?

This only happens if the test failed anyway, so I want to make sure it gets
printed even if someone set a different log level.

[dmazzoni, 2015-12-10 18:42:34]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-10 18:42:35]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1505903002/#ps140001
(title: "Suppress lsan")

[commit-bot: I haz the power, 2015-12-10 18:44:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505903002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505903002/140001

[commit-bot: I haz the power, 2015-12-10 20:20:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-10 20:21:52]

Description was changed from

==========
Don't require expectations file for DumpAccessibility tests.

Instead of requiring an expectations file for DumpAccessibilityTree tests,
just skip tests without an expectations file on that platform. Make it easier
to rebaseline expectations by automatically overwriting expectations of
failing tests when the user passes a command-line switch to
content_browsertests: --generate-accessibility-test-expectations

This lets us delete a bunch of empty expectations files and more easily
see at a glance which files are missing expectations for which tests.

BUG=none
==========

to

==========
Don't require expectations file for DumpAccessibility tests.

Instead of requiring an expectations file for DumpAccessibilityTree tests,
just skip tests without an expectations file on that platform. Make it easier
to rebaseline expectations by automatically overwriting expectations of
failing tests when the user passes a command-line switch to
content_browsertests: --generate-accessibility-test-expectations

This lets us delete a bunch of empty expectations files and more easily
see at a glance which files are missing expectations for which tests.

BUG=none

Committed: https://crrev.com/fb8139fb49877ee41f97f6dc9bc29a5b6e52b5a6
Cr-Commit-Position: refs/heads/master@{#364455}
==========

[commit-bot: I haz the power, 2015-12-10 20:21:53]

Patchset 8 (id:??) landed as
https://crrev.com/fb8139fb49877ee41f97f6dc9bc29a5b6e52b5a6
Cr-Commit-Position: refs/heads/master@{#364455}