Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(245)

Issue 6856075: Tests mutating URL and not removing test nodes.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years ago by frankban
Modified:
8 years ago
Reviewers:
mp+135670
Visibility:
Public.

Description

Tests mutating URL and not removing test nodes. One of the hotkeys tests mutated the URL. Both tests in that test case also failed when run in isolation, and some variables were unused. Also fixed tests leaving elements in the DOM: as a consequence, "Environmenton excellent provider" was visible at the top of the page after a test run. Now the container used by those tests is removed after each test is run. Also removed an XXX saying that removing the elements in afterEach can be problematic. I've tried to reproduce the erroneus behavior described, running the test suite several times, without success. Anyway, removing in afterEach what was created in beforeEach is something we do everywhere in the tests. So, that XXX confused me a bit, any hint? To avoid flickering in the test page, I tried to hide the nodes used by tests where possible. https://code.launchpad.net/~frankban/juju-gui/bug-1081803-tests-mutate-url/+merge/135670 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 3

Patch Set 2 : Tests mutating URL and not removing test nodes. #

Total comments: 1

Patch Set 3 : Tests mutating URL and not removing test nodes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -34 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M test/test_app.js View 1 2 3 chunks +10 lines, -9 lines 0 comments Download
M test/test_app_hotkeys.js View 1 2 2 chunks +25 lines, -25 lines 0 comments Download

Messages

Total messages: 6
frankban
Please take a look.
8 years ago (2012-11-22 12:53:36 UTC) #1
teknico
Nice cleanup, just a couple minor comments inline. https://codereview.appspot.com/6856075/diff/1/test/test_app.js File test/test_app.js (left): https://codereview.appspot.com/6856075/diff/1/test/test_app.js#oldcode50 test/test_app.js:50: } ...
8 years ago (2012-11-22 15:06:01 UTC) #2
frankban
Thanks for the review Nicola. On 2012/11/22 15:06:01, teknico wrote: > Nice cleanup, just a ...
8 years ago (2012-11-22 15:22:49 UTC) #3
frankban
Please take a look.
8 years ago (2012-11-22 15:23:56 UTC) #4
gary.poster
Really nice cleanups, Francesco. Thank you. Please land it! Gary https://codereview.appspot.com/6856075/diff/5001/test/test_app.js File test/test_app.js (left): https://codereview.appspot.com/6856075/diff/5001/test/test_app.js#oldcode44 ...
8 years ago (2012-11-27 13:32:59 UTC) #5
frankban
8 years ago (2012-11-27 14:41:46 UTC) #6
*** Submitted:

Tests mutating URL and not removing test nodes.

One of the hotkeys tests mutated the URL.
Both tests in that test case also failed when run
in isolation, and some variables were unused.

Also fixed tests leaving elements in the DOM:
as a consequence, "Environmenton excellent provider"
was visible at the top of the page after a test run.
Now the container used by those tests is removed
after each test is run. Also removed an XXX saying
that removing the elements in afterEach can be
problematic. I've tried to reproduce the erroneus
behavior described, running the test suite several
times, without success. Anyway, removing in afterEach
what was created in beforeEach is something we do
everywhere in the tests. So, that XXX confused me 
a bit, any hint?

To avoid flickering in the test page, I tried to
hide the nodes used by tests where possible.

R=
CC=
https://codereview.appspot.com/6856075
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b