Patchwork tests: make --json run-tests option create a valid json file

login
register
mail settings
Submitter Laurent Charignon
Date Jan. 17, 2016, 1:14 a.m.
Message ID <cbd838c46ccaded26306.1452993274@dev5073.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12804/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Laurent Charignon - Jan. 17, 2016, 1:14 a.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1452993245 28800
#      Sat Jan 16 17:14:05 2016 -0800
# Node ID cbd838c46ccaded263066898c09e80fda842cdab
# Parent  443848eece189002c542339dc1cf84f49a94c824
tests: make --json run-tests option create a valid json file

Before this patch, the report.json was not a valid json. This patch adds a test
to make sure that the file can be read with the python json module.
Pierre-Yves David - Jan. 17, 2016, 7:49 p.m.
On 01/16/2016 05:14 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon@fb.com>
> # Date 1452993245 28800
> #      Sat Jan 16 17:14:05 2016 -0800
> # Node ID cbd838c46ccaded263066898c09e80fda842cdab
> # Parent  443848eece189002c542339dc1cf84f49a94c824
> tests: make --json run-tests option create a valid json file
>
> Before this patch, the report.json was not a valid json. This patch adds a test
> to make sure that the file can be read with the python json module.
>
> diff --git a/tests/run-tests.py b/tests/run-tests.py
> --- a/tests/run-tests.py
> +++ b/tests/run-tests.py
> @@ -1740,7 +1740,7 @@
>                                       }
>                               outcome[tc.name] = tres
>                       jsonout = json.dumps(outcome, sort_keys=True, indent=4)
> -                    fp.writelines(("testreport =", jsonout))
> +                    fp.writelines(("{\"testreport\" :", jsonout, "}"))

Oops, good find.

We should probably use json dumps for everything.

Adding "outcome" to a "{"testreport": outcome}" dictionnary and using 
json.dumps on that. The current mix of library and manual general feels 
strange to me.

Cheers,
Matt Mackall - Jan. 18, 2016, 3:08 a.m.
On Sun, 2016-01-17 at 11:49 -0800, Pierre-Yves David wrote:
> 
> On 01/16/2016 05:14 PM, Laurent Charignon wrote:
> > # HG changeset patch
> > # User Laurent Charignon <lcharignon@fb.com>
> > # Date 1452993245 28800
> > #      Sat Jan 16 17:14:05 2016 -0800
> > # Node ID cbd838c46ccaded263066898c09e80fda842cdab
> > # Parent  443848eece189002c542339dc1cf84f49a94c824
> > tests: make --json run-tests option create a valid json file
> > 
> > Before this patch, the report.json was not a valid json. This patch adds a
> > test
> > to make sure that the file can be read with the python json module.
> > 
> > diff --git a/tests/run-tests.py b/tests/run-tests.py
> > --- a/tests/run-tests.py
> > +++ b/tests/run-tests.py
> > @@ -1740,7 +1740,7 @@
> >                                       }
> >                               outcome[tc.name] = tres
> >                       jsonout = json.dumps(outcome, sort_keys=True,
> > indent=4)
> > -                    fp.writelines(("testreport =", jsonout))
> > +                    fp.writelines(("{\"testreport\" :", jsonout, "}"))
>
> Oops, good find.
> 
> We should probably use json dumps for everything.
> 
> Adding "outcome" to a "{"testreport": outcome}" dictionnary and using 
> json.dumps on that. The current mix of library and manual general feels 
> strange to me.

Is there any advantage to wrapping the whole thing in another dict?

-- 
Mathematics is the supreme nostalgia of our time.
Pierre-Yves David - Jan. 18, 2016, 3:34 a.m.
On 01/17/2016 07:08 PM, Matt Mackall wrote:
> On Sun, 2016-01-17 at 11:49 -0800, Pierre-Yves David wrote:
>>
>> On 01/16/2016 05:14 PM, Laurent Charignon wrote:
>>> # HG changeset patch
>>> # User Laurent Charignon <lcharignon@fb.com>
>>> # Date 1452993245 28800
>>> #      Sat Jan 16 17:14:05 2016 -0800
>>> # Node ID cbd838c46ccaded263066898c09e80fda842cdab
>>> # Parent  443848eece189002c542339dc1cf84f49a94c824
>>> tests: make --json run-tests option create a valid json file
>>>
>>> Before this patch, the report.json was not a valid json. This patch adds a
>>> test
>>> to make sure that the file can be read with the python json module.
>>>
>>> diff --git a/tests/run-tests.py b/tests/run-tests.py
>>> --- a/tests/run-tests.py
>>> +++ b/tests/run-tests.py
>>> @@ -1740,7 +1740,7 @@
>>>                                        }
>>>                                outcome[tc.name] = tres
>>>                        jsonout = json.dumps(outcome, sort_keys=True,
>>> indent=4)
>>> -                    fp.writelines(("testreport =", jsonout))
>>> +                    fp.writelines(("{\"testreport\" :", jsonout, "}"))
>>
>> Oops, good find.
>>
>> We should probably use json dumps for everything.
>>
>> Adding "outcome" to a "{"testreport": outcome}" dictionnary and using
>> json.dumps on that. The current mix of library and manual general feels
>> strange to me.
>
> Is there any advantage to wrapping the whole thing in another dict?

I can easily imagine generic data about the test running process (total 
time, revision used for the test, load average, etc).

I think it is safer to keep to the current structure (providing it is fixed)

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -1740,7 +1740,7 @@ 
                                     }
                             outcome[tc.name] = tres
                     jsonout = json.dumps(outcome, sort_keys=True, indent=4)
-                    fp.writelines(("testreport =", jsonout))
+                    fp.writelines(("{\"testreport\" :", jsonout, "}"))
                 finally:
                     fp.close()
 
diff --git a/tests/test-run-tests.t b/tests/test-run-tests.t
--- a/tests/test-run-tests.t
+++ b/tests/test-run-tests.t
@@ -524,9 +524,9 @@ 
   # Ran 2 tests, 1 skipped, 0 warned, 1 failed.
   python hash seed: * (glob)
   [1]
-
+  $ python -c "import json; r = json.load(open('report.json'));"
   $ cat report.json
-  testreport ={
+  {"testreport" :{
       "test-failure.t": [\{] (re)
           "csys": "\s*[\d\.]{4,5}", ? (re)
           "cuser": "\s*[\d\.]{4,5}", ? (re)
@@ -554,7 +554,7 @@ 
           "start": "\s*[\d\.]{4,5}", ? (re)
           "time": "\s*[\d\.]{4,5}" (re)
       }
-  } (no-eol)
+  }} (no-eol)
 
 Test that failed test accepted through interactive are properly reported:
 
@@ -574,7 +574,7 @@ 
   # Ran 2 tests, 1 skipped, 0 warned, 0 failed.
 
   $ cat report.json
-  testreport ={
+  {"testreport" :{
       "test-failure.t": [\{] (re)
           "csys": "\s*[\d\.]{4,5}", ? (re)
           "cuser": "\s*[\d\.]{4,5}", ? (re)
@@ -602,7 +602,7 @@ 
           "start": "\s*[\d\.]{4,5}", ? (re)
           "time": "\s*[\d\.]{4,5}" (re)
       }
-  } (no-eol)
+  }} (no-eol)
   $ mv backup test-failure.t
 
 #endif