Launchpad itself

Merge lp:~mars/launchpad/turn-on-windmill-debug-logging into lp:launchpad

  1. turn-on-windmill-debug-logging
  2. Merge into devel
Proposed by Māris Fogels
Status: Merged
Approved by: Māris Fogels
Approved revision: no longer in the source branch.
Merged at revision: 10846
Proposed branch: lp:~mars/launchpad/turn-on-windmill-debug-logging
Merge into: lp:launchpad
Diff against target: 141 lines (+72/-25)
3 files modified
lib/canonical/config/schema-lazr.conf (+8/-1)
lib/canonical/testing/__init__.py (+0/-4)
lib/canonical/testing/layers.py (+64/-20)
To merge this branch: bzr merge lp:~mars/launchpad/turn-on-windmill-debug-logging
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Review via email: mp+24839@code.launchpad.net

Commit message

Make the windmill test suite log debug messages to disk.

Description of the change

Hi,

This branch configures the windmill test suite to log debug messages to a file on disk.

Drive-by changes include removal of a long-released XXX, extraction of the windmill suite setup steps into their own methods, and a comment typo.

I manually tested this change to ensure that the suite still runs, and that the windmill log file is being overwritten properly. I also ensured that the logging framework is reset when the layer finishes.

Test command: bin/test -t test_title_inline_edit
Lint: none

Maris

To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

<rockstar> mars, does reset_logging undo all of this logger buggery you're doing here?
<mars> rockstar, it does
<mars> rockstar, BaseLayer calls it as well, but I did this because it is more visible and explicit
<rockstar> mars, what does safe_hasattr do?
<mars> rockstar, doesn't eat exceptions. Builtin hasattr does (fun fun!)
<rockstar> mars, why not just use getattr?
<mars> rockstar, what line?
<mars> 96
<rockstar> mars, yes.
<mars> rockstar, it might be because hasattr will tell you that an attribute exists. getattr may return None, but you can't tell if that is the attribute's value, or the default argument.
<mars> or the default argument to getattr()
<rockstar> mars, okay. As long as you feel you're using the right call here, I'm happy with that.
<mars> rockstar, not my code, so it should work :)
<mars> rockstar, that is one of the code blocks I extracted into its own method
<mars> personal choice: I dislike 300 and 400 line method definitions, even if they are linear.
<mars> if there are comments that say "# now we do this" followed by "# then we do that", then it is pretty good sign that you can extract a method
<mars> but, like I said, that's personal preference
<rockstar> mars, I think you've made the code clearer by breaking it up.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2010-05-06 20:08:41 +0000
+++ lib/canonical/config/schema-lazr.conf 2010-05-11 16:05:41 +0000
@@ -1929,8 +1929,15 @@
1929[vhost.feeds]1929[vhost.feeds]
19301930
19311931
1932[windmill]
1933# The location of the windmill server debug logs. This will be written to
1934# upon execution of the test suite. Each test run overwrites the previous
1935# log's contents.
1936debug_log: /var/tmp/windmill-testrunner.log
1937
1938
1932# Stubed Key server for test proposes, it's able to server1939# Stubed Key server for test proposes, it's able to server
1933# in SKS fortmat, a restricted set of keys. (fixed address at1940# in SKS format, a restricted set of keys. (fixed address at
1934# localhost:11371)1941# localhost:11371)
1935[zeca]1942[zeca]
1936# Directory to be created to store the pre-installed key-files1943# Directory to be created to store the pre-installed key-files
19371944
=== modified file 'lib/canonical/testing/__init__.py'
--- lib/canonical/testing/__init__.py 2009-08-10 22:08:05 +0000
+++ lib/canonical/testing/__init__.py 2010-05-11 16:05:41 +0000
@@ -32,10 +32,6 @@
32 """Reset the logging system back to defaults32 """Reset the logging system back to defaults
3333
34 Currently, defaults means 'the way the Z3 testrunner sets it up'34 Currently, defaults means 'the way the Z3 testrunner sets it up'
35
36 XXX: StuartBishop 2006-03-08 bug=39877:
37 We need isolation enforcement so that an error will be raised and
38 the test run stop if a test fails to reset the logging system.
39 """35 """
40 # Remove all handlers from non-root loggers, and remove the loggers too.36 # Remove all handlers from non-root loggers, and remove the loggers too.
41 loggerDict = logging.Logger.manager.loggerDict37 loggerDict = logging.Logger.manager.loggerDict
4238
=== modified file 'lib/canonical/testing/layers.py'
--- lib/canonical/testing/layers.py 2010-04-23 18:53:53 +0000
+++ lib/canonical/testing/layers.py 2010-05-11 16:05:41 +0000
@@ -1870,26 +1870,13 @@
1870 # base_url. With no base_url, we can't create the config1870 # base_url. With no base_url, we can't create the config
1871 # file windmill needs.1871 # file windmill needs.
1872 return1872 return
1873 # If we're running in a bin/test sub-process, sys.stdin is1873
1874 # replaced by FakeInputContinueGenerator, which doesn't have a1874 cls._fixStandardInputFileno()
1875 # fileno method. When Windmill starts Firefox,1875 cls._configureWindmillLogging()
1876 # sys.stdin.fileno() is called, so we add such a method here, to1876 cls._configureWindmillStartup()
1877 # prevent it from breaking. By returning None, we should ensure1877
1878 # that it doesn't try to use the return value for anything.1878 # Tell windmill to start its browser and server. Our testrunner will
1879 if not safe_hasattr(sys.stdin, 'fileno'):1879 # keep going, passing commands to the server for execution.
1880 assert isinstance(sys.stdin, FakeInputContinueGenerator), (
1881 "sys.stdin (%r) doesn't have a fileno method." % sys.stdin)
1882 sys.stdin.fileno = lambda: None
1883 # Windmill needs a config file on disk.
1884 config_text = dedent("""\
1885 START_FIREFOX = True
1886 TEST_URL = '%s'
1887 """ % cls.base_url)
1888 cls.config_file = tempfile.NamedTemporaryFile(suffix='.py')
1889 cls.config_file.write(config_text)
1890 # Flush the file so that windmill can read it.
1891 cls.config_file.flush()
1892 os.environ['WINDMILL_CONFIG_FILE'] = cls.config_file.name
1893 cls.shell_objects = start_windmill()1880 cls.shell_objects = start_windmill()
18941881
1895 # Patch the config to provide the port number and not use https.1882 # Patch the config to provide the port number and not use https.
@@ -1916,6 +1903,7 @@
1916 # Close the file so that it gets deleted.1903 # Close the file so that it gets deleted.
1917 cls.config_file.close()1904 cls.config_file.close()
1918 config.reloadConfig()1905 config.reloadConfig()
1906 reset_logging()
19191907
1920 @classmethod1908 @classmethod
1921 @profiled1909 @profiled
@@ -1924,3 +1912,59 @@
1924 # belong to Windmill, which will be cleaned up on layer1912 # belong to Windmill, which will be cleaned up on layer
1925 # tear down.1913 # tear down.
1926 BaseLayer.disable_thread_check = True1914 BaseLayer.disable_thread_check = True
1915
1916 @classmethod
1917 def _fixStandardInputFileno(cls):
1918 """Patch the STDIN fileno so Windmill doesn't break."""
1919 # If we're running in a bin/test sub-process, sys.stdin is
1920 # replaced by FakeInputContinueGenerator, which doesn't have a
1921 # fileno method. When Windmill starts Firefox,
1922 # sys.stdin.fileno() is called, so we add such a method here, to
1923 # prevent it from breaking. By returning None, we should ensure
1924 # that it doesn't try to use the return value for anything.
1925 if not safe_hasattr(sys.stdin, 'fileno'):
1926 assert isinstance(sys.stdin, FakeInputContinueGenerator), (
1927 "sys.stdin (%r) doesn't have a fileno method." % sys.stdin)
1928 sys.stdin.fileno = lambda: None
1929
1930 @classmethod
1931 def _configureWindmillLogging(cls):
1932 """Override the default windmill log handling."""
1933 if not config.windmill.debug_log:
1934 return
1935
1936 # Add a new log handler to capture all of the windmill testrunner
1937 # output. This overrides windmill's own log handling, which we do not
1938 # have direct access to.
1939 # We'll overwrite the previous log contents to keep the disk usage
1940 # low, and because the contents are only meant as an in-situ debugging
1941 # aid.
1942 filehandler = logging.FileHandler(config.windmill.debug_log, mode='w')
1943 filehandler.setLevel(logging.NOTSET)
1944 filehandler.setFormatter(
1945 logging.Formatter(
1946 "%(asctime)s - %(name)s - %(levelname)s - %(message)s"))
1947 logging.getLogger('windmill').addHandler(filehandler)
1948
1949 # Make sure that everything sent to the windmill logger is captured.
1950 # This works because windmill configures the root logger for its
1951 # purposes, and we are pre-empting that by inserting a new logger one
1952 # level higher in the logger chain.
1953 logging.getLogger('windmill').setLevel(logging.NOTSET)
1954
1955 @classmethod
1956 def _configureWindmillStartup(cls):
1957 """Pass our startup parameters to the windmill server."""
1958 # Windmill needs a config file on disk to load its settings from.
1959 # There is no way to directly pass settings to the windmill test
1960 # driver from out here.
1961 config_text = dedent("""\
1962 START_FIREFOX = True
1963 TEST_URL = '%s'
1964 CONSOLE_LOG_LEVEL = %d
1965 """ % (cls.base_url, logging.NOTSET))
1966 cls.config_file = tempfile.NamedTemporaryFile(suffix='.py')
1967 cls.config_file.write(config_text)
1968 # Flush the file so that windmill can read it.
1969 cls.config_file.flush()
1970 os.environ['WINDMILL_CONFIG_FILE'] = cls.config_file.name