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
1=== modified file 'lib/canonical/config/schema-lazr.conf'
2--- lib/canonical/config/schema-lazr.conf 2010-05-06 20:08:41 +0000
3+++ lib/canonical/config/schema-lazr.conf 2010-05-11 16:05:41 +0000
4@@ -1929,8 +1929,15 @@
5 [vhost.feeds]
6
7
8+[windmill]
9+# The location of the windmill server debug logs. This will be written to
10+# upon execution of the test suite. Each test run overwrites the previous
11+# log's contents.
12+debug_log: /var/tmp/windmill-testrunner.log
13+
14+
15 # Stubed Key server for test proposes, it's able to server
16-# in SKS fortmat, a restricted set of keys. (fixed address at
17+# in SKS format, a restricted set of keys. (fixed address at
18 # localhost:11371)
19 [zeca]
20 # Directory to be created to store the pre-installed key-files
21
22=== modified file 'lib/canonical/testing/__init__.py'
23--- lib/canonical/testing/__init__.py 2009-08-10 22:08:05 +0000
24+++ lib/canonical/testing/__init__.py 2010-05-11 16:05:41 +0000
25@@ -32,10 +32,6 @@
26 """Reset the logging system back to defaults
27
28 Currently, defaults means 'the way the Z3 testrunner sets it up'
29-
30- XXX: StuartBishop 2006-03-08 bug=39877:
31- We need isolation enforcement so that an error will be raised and
32- the test run stop if a test fails to reset the logging system.
33 """
34 # Remove all handlers from non-root loggers, and remove the loggers too.
35 loggerDict = logging.Logger.manager.loggerDict
36
37=== modified file 'lib/canonical/testing/layers.py'
38--- lib/canonical/testing/layers.py 2010-04-23 18:53:53 +0000
39+++ lib/canonical/testing/layers.py 2010-05-11 16:05:41 +0000
40@@ -1870,26 +1870,13 @@
41 # base_url. With no base_url, we can't create the config
42 # file windmill needs.
43 return
44- # If we're running in a bin/test sub-process, sys.stdin is
45- # replaced by FakeInputContinueGenerator, which doesn't have a
46- # fileno method. When Windmill starts Firefox,
47- # sys.stdin.fileno() is called, so we add such a method here, to
48- # prevent it from breaking. By returning None, we should ensure
49- # that it doesn't try to use the return value for anything.
50- if not safe_hasattr(sys.stdin, 'fileno'):
51- assert isinstance(sys.stdin, FakeInputContinueGenerator), (
52- "sys.stdin (%r) doesn't have a fileno method." % sys.stdin)
53- sys.stdin.fileno = lambda: None
54- # Windmill needs a config file on disk.
55- config_text = dedent("""\
56- START_FIREFOX = True
57- TEST_URL = '%s'
58- """ % cls.base_url)
59- cls.config_file = tempfile.NamedTemporaryFile(suffix='.py')
60- cls.config_file.write(config_text)
61- # Flush the file so that windmill can read it.
62- cls.config_file.flush()
63- os.environ['WINDMILL_CONFIG_FILE'] = cls.config_file.name
64+
65+ cls._fixStandardInputFileno()
66+ cls._configureWindmillLogging()
67+ cls._configureWindmillStartup()
68+
69+ # Tell windmill to start its browser and server. Our testrunner will
70+ # keep going, passing commands to the server for execution.
71 cls.shell_objects = start_windmill()
72
73 # Patch the config to provide the port number and not use https.
74@@ -1916,6 +1903,7 @@
75 # Close the file so that it gets deleted.
76 cls.config_file.close()
77 config.reloadConfig()
78+ reset_logging()
79
80 @classmethod
81 @profiled
82@@ -1924,3 +1912,59 @@
83 # belong to Windmill, which will be cleaned up on layer
84 # tear down.
85 BaseLayer.disable_thread_check = True
86+
87+ @classmethod
88+ def _fixStandardInputFileno(cls):
89+ """Patch the STDIN fileno so Windmill doesn't break."""
90+ # If we're running in a bin/test sub-process, sys.stdin is
91+ # replaced by FakeInputContinueGenerator, which doesn't have a
92+ # fileno method. When Windmill starts Firefox,
93+ # sys.stdin.fileno() is called, so we add such a method here, to
94+ # prevent it from breaking. By returning None, we should ensure
95+ # that it doesn't try to use the return value for anything.
96+ if not safe_hasattr(sys.stdin, 'fileno'):
97+ assert isinstance(sys.stdin, FakeInputContinueGenerator), (
98+ "sys.stdin (%r) doesn't have a fileno method." % sys.stdin)
99+ sys.stdin.fileno = lambda: None
100+
101+ @classmethod
102+ def _configureWindmillLogging(cls):
103+ """Override the default windmill log handling."""
104+ if not config.windmill.debug_log:
105+ return
106+
107+ # Add a new log handler to capture all of the windmill testrunner
108+ # output. This overrides windmill's own log handling, which we do not
109+ # have direct access to.
110+ # We'll overwrite the previous log contents to keep the disk usage
111+ # low, and because the contents are only meant as an in-situ debugging
112+ # aid.
113+ filehandler = logging.FileHandler(config.windmill.debug_log, mode='w')
114+ filehandler.setLevel(logging.NOTSET)
115+ filehandler.setFormatter(
116+ logging.Formatter(
117+ "%(asctime)s - %(name)s - %(levelname)s - %(message)s"))
118+ logging.getLogger('windmill').addHandler(filehandler)
119+
120+ # Make sure that everything sent to the windmill logger is captured.
121+ # This works because windmill configures the root logger for its
122+ # purposes, and we are pre-empting that by inserting a new logger one
123+ # level higher in the logger chain.
124+ logging.getLogger('windmill').setLevel(logging.NOTSET)
125+
126+ @classmethod
127+ def _configureWindmillStartup(cls):
128+ """Pass our startup parameters to the windmill server."""
129+ # Windmill needs a config file on disk to load its settings from.
130+ # There is no way to directly pass settings to the windmill test
131+ # driver from out here.
132+ config_text = dedent("""\
133+ START_FIREFOX = True
134+ TEST_URL = '%s'
135+ CONSOLE_LOG_LEVEL = %d
136+ """ % (cls.base_url, logging.NOTSET))
137+ cls.config_file = tempfile.NamedTemporaryFile(suffix='.py')
138+ cls.config_file.write(config_text)
139+ # Flush the file so that windmill can read it.
140+ cls.config_file.flush()
141+ os.environ['WINDMILL_CONFIG_FILE'] = cls.config_file.name