Add a JSON logging handler to the Kasko integration tests.

Add a JSON logging handler to the Kasko integration tests.

This produces this kind of log files:

[
  {
    "INFO": "integration_test.pyc:Started server on port 60390"
  },
  {
    "INFO": "process.pyc:Starting chromedriver"
  },
  {
    "INFO": "process.pyc:Starting chrome"
  },
  {
    "INFO": "integration_test.pyc:Navigating to Kasko debug URL"
  },
  {
    "INFO": "integration_test.pyc:Waiting for Kasko report"
  },
  {
    "INFO": "kasko_integration_test.py:Test passed successfully!"
  }
]

Committed: https://crrev.com/449a222fa5e282e8f28f28f3c6be0ac566d39761
Cr-Commit-Position: refs/heads/master@{#383757}

[Sébastien Marchand, 2016-03-24 20:00:08]

Patchset #2 (id:20001) has been deleted

[Sébastien Marchand, 2016-03-24 20:01:09]

sebmarchand@chromium.org changed reviewers:
+ chrisha@chromium.org

[Sébastien Marchand, 2016-03-24 20:01:09]

WDYT of this approach ?

[Sébastien Marchand, 2016-03-24 20:02:45]

Description was changed from

==========
Add a JSON logging handler to the Kasko integration tests.
==========

to

==========
Add a JSON logging handler to the Kasko integration tests.

This produces this kind of log files:

[
  {
    "INFO": "integration_test.pyc:Started server on port 60390"
  }, 
  {
    "INFO": "process.pyc:Starting chromedriver"
  }, 
  {
    "INFO": "process.pyc:Starting chrome"
  }, 
  {
    "INFO": "integration_test.pyc:Navigating to Kasko debug URL"
  }, 
  {
    "INFO": "integration_test.pyc:Waiting for Kasko report"
  }, 
  {
    "INFO": "kasko_integration_test.py:Test passed successfully!"
  }
]
==========

[chrisha, 2016-03-24 21:04:29]

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
File chrome/test/kasko/py/kasko/json_logging_handler.py (right):

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:6: """Logging handler used to
do the logging into a JSON file."""
used to log into a JSON file.

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:13: """A logging handler that
propagate the logging into a JSON file."""
forwards log messages into a JSON file.

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:22: json.dump([], f)
This will emit [] to the file, no? You need to manually write a '['.

And in your destructor you need to terminate the JSON file and write a ']'.

Also, maybe just hold an open handle for the whole lifetime of the handler?

[Sébastien Marchand, 2016-03-29 15:13:35]

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
File chrome/test/kasko/py/kasko/json_logging_handler.py (right):

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:6: """Logging handler used to
do the logging into a JSON file."""
On 2016/03/24 21:04:29, chrisha wrote:
> used to log into a JSON file.

Done.

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:13: """A logging handler that
propagate the logging into a JSON file."""
On 2016/03/24 21:04:29, chrisha wrote:
> forwards log messages into a JSON file.

Done.

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:22: json.dump([], f)
On 2016/03/24 21:04:29, chrisha wrote:
> This will emit [] to the file, no? You need to manually write a '['.
> 
> And in your destructor you need to terminate the JSON file and write a ']'.
> 
> Also, maybe just hold an open handle for the whole lifetime of the handler?

Yeah, this emits [] to the file so when we read it in the |emit| function it
gets interpreted as a list. 

I've tried using a destructor but it didn't worked as expected (i.e. it doesn't
get called) and it doesn't seem like the pythonic way to do this. Instead I
re-open the file and I append the log message to the list of messages that it
previously contained.

[chrisha, 2016-03-29 15:25:13]

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
File chrome/test/kasko/py/kasko/json_logging_handler.py (right):

https://codereview.chromium.org/1834773002/diff/40001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:22: json.dump([], f)
On 2016/03/29 15:13:35, Sébastien Marchand wrote:
> On 2016/03/24 21:04:29, chrisha wrote:
> > This will emit [] to the file, no? You need to manually write a '['.
> > 
> > And in your destructor you need to terminate the JSON file and write a ']'.
> > 
> > Also, maybe just hold an open handle for the whole lifetime of the handler?
> 
> Yeah, this emits [] to the file so when we read it in the |emit| function it
> gets interpreted as a list. 
> 
> I've tried using a destructor but it didn't worked as expected (i.e. it
doesn't
> get called) and it doesn't seem like the pythonic way to do this. Instead I
> re-open the file and I append the log message to the list of messages that it
> previously contained.

Ah, okay. I see what you do. You actually rewrite the entire log file with every
log message.

Why not make use of the Handler.Close method to know when the logging handler is
finished? Reading, parsing and rewriting the entire log at each log statement
hardly seems ideal.

https://docs.python.org/2/library/logging.html#handler-objects

[Sébastien Marchand, 2016-03-29 15:46:36]

Dang, I should have spent more time reading this documentation. PTAnL

[chrisha, 2016-03-29 15:50:28]

lgtm with a nit

https://codereview.chromium.org/1834773002/diff/80001/chrome/test/kasko/py/ka...
File chrome/test/kasko/py/kasko/json_logging_handler.py (right):

https://codereview.chromium.org/1834773002/diff/80001/chrome/test/kasko/py/ka...
chrome/test/kasko/py/kasko/json_logging_handler.py:25:
f.write(json.dumps(self.log_messages_))
The documentation says also to be sure to call the parent class version of
'close'.

[Sébastien Marchand, 2016-03-29 15:54:10]

As I said, I should have read the documentation more carefully :)

[Sébastien Marchand, 2016-03-29 15:54:17]

The CQ bit was checked by sebmarchand@chromium.org

[Sébastien Marchand, 2016-03-29 15:54:18]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/1834773002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-03-29 15:54:28]

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

[commit-bot: I haz the power, 2016-03-29 17:36:42]

Description was changed from

==========
Add a JSON logging handler to the Kasko integration tests.

This produces this kind of log files:

[
  {
    "INFO": "integration_test.pyc:Started server on port 60390"
  }, 
  {
    "INFO": "process.pyc:Starting chromedriver"
  }, 
  {
    "INFO": "process.pyc:Starting chrome"
  }, 
  {
    "INFO": "integration_test.pyc:Navigating to Kasko debug URL"
  }, 
  {
    "INFO": "integration_test.pyc:Waiting for Kasko report"
  }, 
  {
    "INFO": "kasko_integration_test.py:Test passed successfully!"
  }
]
==========

to

==========
Add a JSON logging handler to the Kasko integration tests.

This produces this kind of log files:

[
  {
    "INFO": "integration_test.pyc:Started server on port 60390"
  }, 
  {
    "INFO": "process.pyc:Starting chromedriver"
  }, 
  {
    "INFO": "process.pyc:Starting chrome"
  }, 
  {
    "INFO": "integration_test.pyc:Navigating to Kasko debug URL"
  }, 
  {
    "INFO": "integration_test.pyc:Waiting for Kasko report"
  }, 
  {
    "INFO": "kasko_integration_test.py:Test passed successfully!"
  }
]
==========

[commit-bot: I haz the power, 2016-03-29 17:36:43]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-03-29 17:37:57]

Description was changed from

==========
Add a JSON logging handler to the Kasko integration tests.

This produces this kind of log files:

[
  {
    "INFO": "integration_test.pyc:Started server on port 60390"
  }, 
  {
    "INFO": "process.pyc:Starting chromedriver"
  }, 
  {
    "INFO": "process.pyc:Starting chrome"
  }, 
  {
    "INFO": "integration_test.pyc:Navigating to Kasko debug URL"
  }, 
  {
    "INFO": "integration_test.pyc:Waiting for Kasko report"
  }, 
  {
    "INFO": "kasko_integration_test.py:Test passed successfully!"
  }
]
==========

to

==========
Add a JSON logging handler to the Kasko integration tests.

This produces this kind of log files:

[
  {
    "INFO": "integration_test.pyc:Started server on port 60390"
  },
  {
    "INFO": "process.pyc:Starting chromedriver"
  },
  {
    "INFO": "process.pyc:Starting chrome"
  },
  {
    "INFO": "integration_test.pyc:Navigating to Kasko debug URL"
  },
  {
    "INFO": "integration_test.pyc:Waiting for Kasko report"
  },
  {
    "INFO": "kasko_integration_test.py:Test passed successfully!"
  }
]

Committed: https://crrev.com/449a222fa5e282e8f28f28f3c6be0ac566d39761
Cr-Commit-Position: refs/heads/master@{#383757}
==========

[commit-bot: I haz the power, 2016-03-29 17:37:58]

Patchset 5 (id:??) landed as
https://crrev.com/449a222fa5e282e8f28f28f3c6be0ac566d39761
Cr-Commit-Position: refs/heads/master@{#383757}