Thanks for the reviews Gary and Benji. Tests on ec2 work without intermittent failures. Tests on LXC run very slowly. We are landing this branch and I will keep on launching test runs on ec2. I will add a card in case I experience further fragilities.
https://codereview.appspot.com/6842074/diff/3001/README.txt File README.txt (right): https://codereview.appspot.com/6842074/diff/3001/README.txt#newcode62 README.txt:62: JUJU_REPOSITORY=/path/to/local/repo ~/bin/jitsu test juju-gui --logdir /tmp On 2012/11/26 21:46:56, gary.poster wrote: > The instructions don't specify what a repo is. It would be nice to clarify > that, and integrate the repo instructions into the functional test setup above. Done. https://codereview.appspot.com/6842074/diff/3001/config/juju-api-improv.conf.template File config/juju-api-improv.conf.template (right): https://codereview.appspot.com/6842074/diff/3001/config/juju-api-improv.conf.template#newcode2 config/juju-api-improv.conf.template:2: author "Juju GUI Peeps" On 2012/11/26 21:36:17, benji wrote: > Should this be "Canonical" or something similarly formal? Done. https://codereview.appspot.com/6842074/diff/3001/config/juju-gui.conf.template File config/juju-gui.conf.template (right): https://codereview.appspot.com/6842074/diff/3001/config/juju-gui.conf.template#newcode10 config/juju-gui.conf.template:10: exec nodejs %(juju_gui_dir)s/server.js On 2012/11/26 21:46:56, gary.poster wrote: > This starts the debug server. Eventually, I think it would be better to have > the default installation use the static production files, found in the build > directory. Of course, later, we will actually want to have releases of the > static files alone. We would probably want three charm config options then: > deploy debug from branch, deploy production from branch, or deploy production > from "release" (a zip file of the build directory). The default would then be > the newest release. > For now, what you have done is probably the right thing to do. The other easy > alternative is to replace line 10 with these 2 lines. > chdir /home/ubuntu/gui/build > exec python -m SimpleHTTPServer 8080 > The downside to that approach is that people will not be able to pass URLs > around. > In the longer term, we would install nginx or apache and configure it to serve > our build, with our index.html as the 404 page (or similar). We agree the charm should start the production server. Created a card with your comments in it: https://bugs.launchpad.net/juju-gui/+bug/1083545 https://codereview.appspot.com/6842074/diff/3001/hooks/start File hooks/start (right): https://codereview.appspot.com/6842074/diff/3001/hooks/start#newcode23 hooks/start:23: CURRENT_DIR = os.getcwd() On 2012/11/26 21:36:17, benji wrote: > Is it safe to assume that the CWD is what we expect it to be? Would it be safer > to base our paths off the current file's location? According to the Juju FAQs the hooks are always executed in the root directory of the charm: https://juju.ubuntu.com/docs/faq.html#what-directory-are-hooks-executed-in https://codereview.appspot.com/6842074/diff/3001/hooks/start#newcode52 hooks/start:52: open_port(8081) On 2012/11/26 21:46:56, gary.poster wrote: > As above. I was wondering why we needed to expose improv until the obvious hit > me in the head like a brick. > Maybe we should include comments to specify which port is for which service? Done. https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test File tests/deploy.test (right): https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test#newcode23 tests/deploy.test:23: except urllib2.URLError as err: On 2012/11/26 21:36:17, benji wrote: > Unfortunately urllib2.urlopen can throw several different kinds of exceptions in > different scenarios. > I suppose we could put a fairly generic "except: Exception" here instead or try > to enumerate them all. I know that the set includes at least HTTPError (from > httplib) and IOError. There are probably many more. > Honestly, do we really think that retrying for 30 seconds will make a material > difference? If not, we could just leave the retries out altogether. After all, > this isn't a long-running process that needs to stay up at all cost. Adding a timeout is needed because of having to wait for the service to be exposed. Unfortunately it seems that we cannot check that using jitsu watch. Catching URLError (and not other exceptions) allows us to do that. We do not want to know if the response status is 200, we only want to know when the port is reachable. Added a comment to the code explaining this. https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test#newcode38 tests/deploy.test:38: '--state', 'started', '--open-port', self.port) On 2012/11/26 21:46:56, gary.poster wrote: > This test failed for me once (see below), and passed for me once. I'm on > Quantal, running Juju tests on a Precise LXC. If you don't see intermittent > behavior, I'm OK with proceeding, but we should do so with caution--if anyone > else reports intermittent problems, or if I see this again, we should dig in. > ====================================================================== > ERROR: test_improv (__main__.DeployTest) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "./deploy.test", line 38, in setUp > '--state', 'started', '--open-port', self.port) > File "/usr/lib/python2.7/dist-packages/shelltoolbox/__init__.py", line 133, in > callable_command > return run(*all_args) > File "/usr/lib/python2.7/dist-packages/shelltoolbox/__init__.py", line 452, in > run > raise exception > CalledProcessError: Command '['jitsu', 'watch', '--failfast', 'juju-gui', > '--state', 'started', '--open-port', '8888']' returned non-zero exit status 1 > ---------------------------------------------------------------------- > Ran 1 test in 303.993s > FAILED (errors=1) This one seems a test runner failure. We have also experienced timeout errors when running the tests using LXC. We added a note in README with instructions about how to increase the jitsu test timeout. https://codereview.appspot.com/6842074/diff/3001/tests/deploy.test#newcode57 tests/deploy.test:57: unittest.main(verbosity=2) On 2012/11/26 21:36:17, benji wrote: > Doesn't the test runner inspect sys.argv so you can pass switches? Imposing a > non-standard default is unwarranted. It seems not possible to pass arguments through the jitsu test command to the test runner. A verbosity of 2 seems to us a good default to have enough feedback about what is going on. https://codereview.appspot.com/6842074/ -- https://code.launchpad.net/~frankban/charms/precise/juju-gui/juju-gui/+merge/135213 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~frankban/charms/precise/juju-gui/juju-gui into lp:~juju-gui/charms/precise/juju-gui/trunk. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
