Merge lp://qastaging/~vila/selenium-simple-test/1161336-hookable-browser into lp://qastaging/selenium-simple-test

Proposed by Vincent Ladeuil
Status: Merged
Approved by: Corey Goldberg
Approved revision: 373
Merged at revision: 375
Proposed branch: lp://qastaging/~vila/selenium-simple-test/1161336-hookable-browser
Merge into: lp://qastaging/selenium-simple-test
Diff against target: 1298 lines (+315/-296)
12 files modified
src/sst/actions.py (+66/-182)
src/sst/bmobproxy.py (+1/-0)
src/sst/command.py (+13/-10)
src/sst/context.py (+1/-3)
src/sst/runtests.py (+175/-55)
src/sst/scripts/remote.py (+13/-8)
src/sst/scripts/run.py (+12/-5)
src/sst/selftests/test_two_methods.py (+0/-1)
src/sst/tests/test_runtests_get_suites.py (+10/-19)
src/sst/tests/test_sst_run.py (+0/-4)
src/sst/tests/test_sst_script_test_case.py (+23/-8)
src/sst/tests/test_sst_test_case.py (+1/-1)
To merge this branch: bzr merge lp://qastaging/~vila/selenium-simple-test/1161336-hookable-browser
Reviewer Review Type Date Requested Status
Corey Goldberg (community) Approve
Review via email: mp+158395@code.qastaging.launchpad.net

Commit message

Make the selenium webdriver object a test attribute allowing tests to override it to suit their needs

Description of the change

This fixes bug #1161336 by making the selenium webdriver object a test attribute and as such remove the global variables 'browser' and 'browsermob_proxy' used in actions.py.

The new design introduces browser factories classes that can create the right type of browser.

Since some browser properties can be set by the tests themselves, the browser creation must be delayed until the test setup. Yet, some of these options can also be provided from the command line so there was a need to collect them, hence the factory.

The BrowserFactory can collect browser properties (is javascript enabled or the like) so they can be provided to the test setup.

This design simplify the functions used to create the tests (runtests, get_suites, get_suite, get_case) from the scripts since they can carry only the browser factory instead of all browser options.

Additionnally, this allowed some more simplification as some options are browser specific (most notably, the ones required by sst-remote doesn't have to be carried when using sst-run and vice-versa).

For lack of a better way, the browser factory is also responsible for handling the browsermob_proxy (missing tests). This allowed removing the 'browsermob_proxy' global variable too.

From there, a few modifications had to be made in actions.py:

- all access to the browser objects are now done via the '_test' global variable which each test set during its setup, this allowed removing the 'browser' global variable,

- the actions 'start' and 'stop' have been removed (their core is now part of the test classes). Since sst guarantee that the browser is handled automatically, there is no point in allowing these actions to a user,

- the http capture features have been factorized and moved to the browser_factory object (may be a bit controversial but given their implementation that was the best fit),

Overall, this refactoring could have been simpler if we had a better test
coverage. Namely, the following areas were not covered:

* no tests for sst-remote (I'm unclear about which feature is provided here and how to set it up),

* there are a bunch of _make_results_dir calls in actions.py which shouldn't be necessary, _make_results_dir is already called during test setUp() (unless that one is wrong ?),

* no tests for .har files (would probably help remove any reamining controversy around where the methods should be defined ;)

Final note, I merged the test from https://code.launchpad.net/~elopio/selenium-simple-test/fix1167680-dont_populate_context/+merge/158267 after discussing with elopio.

To post a comment you must log in.
372. By Vincent Ladeuil

Add IeFactory following elopio review

Revision history for this message
Corey Goldberg (coreygoldberg) wrote :

I like this.

I'd really like to remove browsermob integration feature in it's entirety as it would much simplify things. (including this new design)... will do so in future branch so this doesn't block progress on other projects.

Revision history for this message
Corey Goldberg (coreygoldberg) wrote :

alternate drivers not working (Chrome, PhantomJS).

--------
$./ci.sh --bootstrap --acceptance Chrome

[snip]

starting SST...
--------------------------------------------------------------
waiting for django to come up...
django found. continuing...

starting virtual display...
--------------------------------------------------------------

killing django...

stopping virtual display...
Traceback (most recent call last):
  File "./sst-run", line 10, in <module>
    run.main()
  File "./src/sst/scripts/run.py", line 105, in main
    browsermob_process),
TypeError: object.__new__() takes no parameters
----------------------------------
--------

373. By Vincent Ladeuil

Fix constructor for BrowserFactory as it's mandatory to support alternate browsers.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> alternate drivers not working (Chrome, PhantomJS).

Fixed.

Or rather, put back in the state where the acceptance are failing like they are currently on trunk:

- Chrome: FAILED (failures=2, errors=1)
- PhantomJS: FAILED (failures=1, errors=5)

Revision history for this message
Vincent Ladeuil (vila) wrote :

Filed http://pad.lv/1168268 and http://pad.lv/1168274 to separate the issues and unblock this proposal.

Revision history for this message
Corey Goldberg (coreygoldberg) wrote :

thanks.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches