| 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
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,
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.
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