When running DM, write test failures to json.

When running DM, write test failures to json.

Add skiatest::Failure to keep track of data about a test failure.

Reporter::reportFailed and ::onReportFailed now take Failure as a
parameter. This allows the implementation to treat the failure as it
wishes. Provide a helper to format the failure the same as prior to
the change.

Update the macros for calling reportFailed (REPORTER_ASSERT etc) to
create a Failure object.

Convert a direct call to reportFailed to the macro ERRORF.

Write Failures to Json.
Sample output when running dm on the dummy test crrev.com/705723004:
{
   "test_results" : {
      "failures" : [
         {
            "condition" : "0 > 3",
            "file_name" : "../../tests/DummyTest.cpp",
            "line_no" : 10,
            "message" : ""
         },
         {
            "condition" : "false",
            "file_name" : "../../tests/DummyTest.cpp",
            "line_no" : 4,
            "message" : ""
         },
         {
            "condition" : "1 == 3",
            "file_name" : "../../tests/DummyTest.cpp",
            "line_no" : 5,
            "message" : "I can too count!"
         },
         {
            "condition" : "",
            "file_name" : "../../tests/DummyTest.cpp",
            "line_no" : 6,
            "message" : "seven is 7"
         },
         {
            "condition" : "1 == 3",
            "file_name" : "../../tests/DummyTest.cpp",
            "line_no" : 14,
            "message" : "I can too count!"
         }
      ]
   }
}

Report all of the failures from one test.
Previously, if one test had multiple failures, only one was reportered.
e.g:

Failures:
  test Dummy: ../../tests/DummyTest.cpp:6   seven is 7
  test Dummy2: ../../tests/DummyTest.cpp:10 0 > 3
  test Dummy3: ../../tests/DummyTest.cpp:14 I can too count!: 1 == 3
3 failures.

Now, we get all the messages:

Failures:
  test Dummy: ../../tests/DummyTest.cpp:4   false
        ../../tests/DummyTest.cpp:5 I can too count!: 1 == 3
        ../../tests/DummyTest.cpp:6 seven is 7
  test Dummy2: ../../tests/DummyTest.cpp:10 0 > 3
  test Dummy3: ../../tests/DummyTest.cpp:14 I can too count!: 1 == 3
3 failures.

(Note that we still state "3 failures" because 3 DM::Tasks failed.)

BUG=skia:3082
BUG=skia:2454
Committed: https://skia.googlesource.com/skia/+/0ee2627026370bbb25e4efda490e14e468ac86fb

[scroggo, 2014-11-05 20:40:17]

scroggo@google.com changed reviewers:
+ jcgregorio@google.com, mtklein@google.com

[scroggo, 2014-11-05 20:40:18]

Joe, I am including you because it is my understanding that you're the one who
deals with JSON output. I'd like for that output to be used to determine that
tests failed (on Android TradeFed where we don't know the return value of
tests). As mentioned inline, I don't feel strongly about the format.

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp
File dm/DMJsonWriter.cpp (right):

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp#newcode64
dm/DMJsonWriter.cpp:64: const char* test_results = "test_results";
This block writes out the test results in the format listed in the change
description. I'm not attached to that format, so if something would be better
for the bots or a human to read, please let me know.

https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h
File dm/DMTestTask.h (right):

https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h#newcode33
dm/DMTestTask.h:33: fFailure.append(newFailure);
This change in behavior is orthogonal to the rest of my change. Was it
deliberate that DM overwrites previous failures? Most of our tests include
multiple REPORTER_ASSERTs, and I think it is preferable to be able to see them
all.

[mtklein, 2014-11-06 15:24:05]

I have a bunch of nits, but this looks great.  Very excited to see it land.

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp
File dm/DMJsonWriter.cpp (right):

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp#newcode64
dm/DMJsonWriter.cpp:64: const char* test_results = "test_results";
It's probably OK to just write "test_results" where it's used. :)

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp#newcode64
dm/DMJsonWriter.cpp:64: const char* test_results = "test_results";
On 2014/11/05 20:40:17, scroggo wrote:
> This block writes out the test results in the format listed in the change
> description. I'm not attached to that format, so if something would be better
> for the bots or a human to read, please let me know.

You're adding this, so you tell us what's a good format.  No humans or bots
currently look at this, because it doesn't exist. :)

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp#newcode66
dm/DMJsonWriter.cpp:66: root[test_results]["num_failures"] = gFailures.count();
Isn't this redundant?  It's sort of clear from the length of the failures list.

https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h
File dm/DMTestTask.h (right):

https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h#newcode33
dm/DMTestTask.h:33: fFailure.append(newFailure);
On 2014/11/05 20:40:17, scroggo wrote:
> This change in behavior is orthogonal to the rest of my change. Was it
> deliberate that DM overwrites previous failures? Most of our tests include
> multiple REPORTER_ASSERTs, and I think it is preferable to be able to see them
> all.

Nope.  This seems like a good change to me.  Might be better to store an
SkTArray<SkString> fFailures, but this seems like a good thing to land right
away.

https://codereview.chromium.org/694703005/diff/1/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/694703005/diff/1/tests/Test.h#newcode24
tests/Test.h:24: SkString fileName;
Given that you know filename and condition are going to be compile-time constant
const char*, why not hold onto them as const char*?  Fewer refs, copies,
c_strs(), etc.

https://codereview.chromium.org/694703005/diff/1/tests/Test.h#newcode30
tests/Test.h:30: static void GetFailureString(SkString* result, const Failure&
failure) {
Seems like this is simpler as SkString getFailureString() const ?

https://codereview.chromium.org/694703005/diff/1/tests/Test.h#newcode164
tests/Test.h:164: SkString(),                       \
Might want something like "<ERRORF>" or "unconditional" here?

[jcgregorio, 2014-11-06 15:45:12]

On 2014/11/05 20:40:18, scroggo wrote:
> Joe, I am including you because it is my understanding that you're the one who
> deals with JSON output. I'd like for that output to be used to determine that
> tests failed (on Android TradeFed where we don't know the return value of
> tests). As mentioned inline, I don't feel strongly about the format.

The format looks good, but I would want to add similar information that we
add to the DM and nanobench JSON output if that's available when it's called.
For example:

{
   "build_number" : "394",
   "gitHash" : "cdccd75925185b25972b8b80f8492bd9d263028d",
   "key" : {
      "arch" : "x86_64",
      "configuration" : "Release",
      "extra_config" : "Shared",
      "gpu" : "NoGPU",
      "model" : "GCE",
      "os" : "Ubuntu13.10"
   },



> 
> https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp
> File dm/DMJsonWriter.cpp (right):
> 
> https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp#newcode64
> dm/DMJsonWriter.cpp:64: const char* test_results = "test_results";
> This block writes out the test results in the format listed in the change
> description. I'm not attached to that format, so if something would be better
> for the bots or a human to read, please let me know.
> 
> https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h
> File dm/DMTestTask.h (right):
> 
> https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h#newcode33
> dm/DMTestTask.h:33: fFailure.append(newFailure);
> This change in behavior is orthogonal to the rest of my change. Was it
> deliberate that DM overwrites previous failures? Most of our tests include
> multiple REPORTER_ASSERTs, and I think it is preferable to be able to see them
> all.

[mtklein, 2014-11-06 15:53:17]

> The format looks good, but I would want to add similar information that we
> add to the DM and nanobench JSON output if that's available when it's called.
> For example:
> 
> {
>    "build_number" : "394",
>    "gitHash" : "cdccd75925185b25972b8b80f8492bd9d263028d",
>    "key" : {
>       "arch" : "x86_64",
>       "configuration" : "Release",
>       "extra_config" : "Shared",
>       "gpu" : "NoGPU",
>       "model" : "GCE",
>       "os" : "Ubuntu13.10"
>    },

Correct me if I'm wrong, but this is going _in_ the dm.json, so all that data is
already there.  We're adding "test_failures": [ ... ] as a sibling of "results":
[ ... ].

[jcgregorio, 2014-11-06 15:57:35]

On 2014/11/06 15:53:17, mtklein wrote:
> > The format looks good, but I would want to add similar information that we
> > add to the DM and nanobench JSON output if that's available when it's
called.
> > For example:
> > 
> > {
> >    "build_number" : "394",
> >    "gitHash" : "cdccd75925185b25972b8b80f8492bd9d263028d",
> >    "key" : {
> >       "arch" : "x86_64",
> >       "configuration" : "Release",
> >       "extra_config" : "Shared",
> >       "gpu" : "NoGPU",
> >       "model" : "GCE",
> >       "os" : "Ubuntu13.10"
> >    },
> 
> Correct me if I'm wrong, but this is going _in_ the dm.json, so all that data
is
> already there.  We're adding "test_failures": [ ... ] as a sibling of
"results":
> [ ... ].

If that's the case then that's great.

LGTM

[scroggo, 2014-11-06 16:50:08]

Patchset #3 (id:40001) has been deleted

[scroggo, 2014-11-06 16:52:33]

On 2014/11/06 15:53:17, mtklein wrote:
> Correct me if I'm wrong, but this is going _in_ the dm.json, so all that data
is
> already there.  We're adding "test_failures": [ ... ] as a sibling of
"results":
> [ ... ].

That is correct.

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp
File dm/DMJsonWriter.cpp (right):

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp#newcode64
dm/DMJsonWriter.cpp:64: const char* test_results = "test_results";
On 2014/11/06 15:24:05, mtklein wrote:
> It's probably OK to just write "test_results" where it's used. :)

I was being cautious since I used it twice. I've removed one of those uses, so
there's no reason for it anymore.

https://codereview.chromium.org/694703005/diff/1/dm/DMJsonWriter.cpp#newcode66
dm/DMJsonWriter.cpp:66: root[test_results]["num_failures"] = gFailures.count();
On 2014/11/06 15:24:05, mtklein wrote:
> Isn't this redundant?  It's sort of clear from the length of the failures
list.

It is redundant, but I thought maybe it's easier to have one place to look than
to count? That said, what we really care about is that there exists some
failure, rather than the precise number. I'll remove.

https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h
File dm/DMTestTask.h (right):

https://codereview.chromium.org/694703005/diff/1/dm/DMTestTask.h#newcode33
dm/DMTestTask.h:33: fFailure.append(newFailure);
On 2014/11/06 15:24:05, mtklein wrote:
> On 2014/11/05 20:40:17, scroggo wrote:
> > This change in behavior is orthogonal to the rest of my change. Was it
> > deliberate that DM overwrites previous failures? Most of our tests include
> > multiple REPORTER_ASSERTs, and I think it is preferable to be able to see
them
> > all.
> 
> Nope.  This seems like a good change to me.  Might be better to store an
> SkTArray<SkString> fFailures, but this seems like a good thing to land right
> away.

Yeah, I considered doing that, but I wasn't sure the best way to plumb it
through. If it's alright by you (and it sounds like it is), I'll leave that as a
TODO.

https://codereview.chromium.org/694703005/diff/1/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/694703005/diff/1/tests/Test.h#newcode24
tests/Test.h:24: SkString fileName;
On 2014/11/06 15:24:05, mtklein wrote:
> Given that you know filename and condition are going to be compile-time
constant
> const char*, why not hold onto them as const char*?  Fewer refs, copies,
> c_strs(), etc.

To be fair, someone *could* skip the macro and create their own... In practice,
though, you're right, and we should discourage skipping the macro. Changed and
added comments.

https://codereview.chromium.org/694703005/diff/1/tests/Test.h#newcode30
tests/Test.h:30: static void GetFailureString(SkString* result, const Failure&
failure) {
On 2014/11/06 15:24:05, mtklein wrote:
> Seems like this is simpler as SkString getFailureString() const ?

Definitely simpler.

FWIW, I almost made it an outside function, since it doesn't deal with any
private data, but I made it a part of Failure for namespacing. Since it's a part
of Failure, I may as well make it a member function.

I'd like to keep passing result in as a parameter to avoid a copy, though.

https://codereview.chromium.org/694703005/diff/1/tests/Test.h#newcode164
tests/Test.h:164: SkString(),                       \
On 2014/11/06 15:24:05, mtklein wrote:
> Might want something like "<ERRORF>" or "unconditional" here?

I'm not sure that adds any more information. The other macros test a condition,
but with ERRORF the condition just happened outside the macro. The caller should
be describing the condition that reached the ERRORF line in the message.

[mtklein, 2014-11-06 17:46:54]

lgtm

https://codereview.chromium.org/694703005/diff/60001/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/694703005/diff/60001/tests/Test.h#newcode173
tests/Test.h:173: NULL, SkString(desc) };           \
Consider "" instead of NULL here, so we never have to worry about NULL checks?

[scroggo, 2014-11-06 20:29:32]

https://codereview.chromium.org/694703005/diff/60001/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/694703005/diff/60001/tests/Test.h#newcode173
tests/Test.h:173: NULL, SkString(desc) };           \
On 2014/11/06 17:46:54, mtklein wrote:
> Consider "" instead of NULL here, so we never have to worry about NULL checks?

Done.

[scroggo, 2014-11-06 20:29:36]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-11-06 20:30:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/694703005/80001

[commit-bot: I haz the power, 2014-11-07 14:07:38]

Committed patchset #4 (id:80001) as 0ee2627026370bbb25e4efda490e14e468ac86fb