Merge lp://qastaging/~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup into lp://qastaging/webbrowser-app

Proposed by Alexandre Abreu
Status: Needs review
Proposed branch: lp://qastaging/~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup
Merge into: lp://qastaging/webbrowser-app
Diff against target: 564 lines (+319/-12)
11 files modified
src/app/webcontainer/WebApp.qml (+2/-0)
src/app/webcontainer/WebViewImplOxide.qml (+35/-9)
src/app/webcontainer/WebViewImplWebkit.qml (+3/-0)
src/app/webcontainer/WebappContainerWebview.qml (+4/-1)
src/app/webcontainer/webapp-container.cpp (+24/-2)
src/app/webcontainer/webapp-container.h (+1/-0)
src/app/webcontainer/webapp-container.qml (+2/-0)
tests/autopilot/webapp_container/tests/__init__.py (+14/-0)
tests/autopilot/webapp_container/tests/fake_servers.py (+84/-0)
tests/autopilot/webapp_container/tests/test_navigation_filtering.py (+92/-0)
tests/autopilot/webapp_container/tests/test_navigation_popup.py (+58/-0)
To merge this branch: bzr merge lp://qastaging/~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup
Reviewer Review Type Date Requested Status
system-apps-ci-bot continuous-integration Needs Fixing
PS Jenkins bot continuous-integration Approve
Ubuntu Phablet Team Pending
Review via email: mp+217811@code.qastaging.launchpad.net

Commit message

Add navigation requested & popup handling AP tests

Description of the change

Add navigation requested & popup handling AP tests

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:509
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/788/
Executed test runs:
    UNSTABLE: http://s-jenkins.ubuntu-ci:8080/job/generic-mediumtests-trusty/5159
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/generic-mediumtests-trusty-touch/4270/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-trusty-amd64-ci/290
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-trusty-armhf-ci/290
        deb: http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-trusty-armhf-ci/290/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-trusty-i386-ci/290
    UNSTABLE: http://s-jenkins.ubuntu-ci:8080/job/autopilot-testrunner-otto-trusty/4433
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/generic-mediumtests-builder-trusty-amd64/5364
        deb: http://s-jenkins.ubuntu-ci:8080/job/generic-mediumtests-builder-trusty-amd64/5364/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/generic-mediumtests-builder-trusty-armhf/4804
        deb: http://s-jenkins.ubuntu-ci:8080/job/generic-mediumtests-builder-trusty-armhf/4804/artifact/work/output/*zip*/output.zip
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/generic-mediumtests-runner-mako/6497/console
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6575/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/788/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:509
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/794/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/5218/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/4297
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-amd64-ci/296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-armhf-ci/296
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-armhf-ci/296/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-trusty-i386-ci/296
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/4485/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/5443
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/5443/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/4883
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/4883/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/6524
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6722

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/794/rebuild

review: Needs Fixing (continuous-integration)
509. By Olivier Tilloy

Work around a recent regression by forcing the OSK to show up when the address bar is being focused. Fixes: 1316057

510. By PS Jenkins bot

Releasing 0.23+14.10.20140505-0ubuntu1

511. By Michael Sheldon

Resolve image URLs beginning with a double slash correctly for context menu items Fixes: 1311626

512. By Adnane Belmadiaf

Enabled passwordEchoEnabled Fixes: 1314251

513. By Olivier Tilloy

Build the models in a separate static lib, and link the unit tests against it.
This speeds up build time by avoiding having to recompile the models’ source for each unit test.

514. By Olivier Tilloy

Handle javascript console messages.

515. By Olivier Tilloy

Escape literal dots in UA override matching regular expressions.

516. By Olivier Tilloy

Enable localStorage by default in the browser. Fixes: 1309673, 1310658

517. By Olivier Tilloy

Ensure that the URL actually changes so that the address bar is updated in case the user has entered a new address that redirects to where she previously was. Fixes: 1306615

518. By Olivier Tilloy

Update bzr ignore rules.

519. By Alberto Mardegan

Split UbuntuWebContext into two different components:
- UbuntuWebContext, which is a WebContext derivative with the UA overrides for Ubuntu
- UbuntuSharedWebContext, which is a singleton for UbuntuWebContext

520. By PS Jenkins bot

Releasing 0.23+14.10.20140505.1-0ubuntu1

521. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

522. By Olivier Tilloy

Enable cross compilation for an ARM target on an X86 host.

523. By Olivier Tilloy

Port autopilot tests to Python 3.

524. By PS Jenkins bot

Releasing 0.23+14.10.20140506-0ubuntu1

525. By Alexandre Abreu

Add capability for single webapps to have specific UA overrides for the website that they serve Fixes: 1245465

526. By PS Jenkins bot

Releasing 0.23+14.10.20140506.1-0ubuntu1

527. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

528. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

529. By Alexandre Abreu

Add --local-webapp-manifest webapp container cli option to simplify the command line in the case of a local manifest.json file definition. This is to become a bit more important now that the manifest support thing like ua overrides that would be beneficial to webapp on touch.

530. By PS Jenkins bot

Releasing 0.23+14.10.20140514.1-0ubuntu1

531. By Michael Sheldon

Add support for downloading images via download manager and content-hub on non-desktop platforms.

532. By Tim Peeters

Push the initial Page on the PageStack.

533. By PS Jenkins bot

Releasing 0.23+14.10.20140515.1-0ubuntu1

534. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

535. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

536. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

537. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

538. By Olivier Tilloy

Remove an extraneous whitespace in the default UA on mobile.

539. By PS Jenkins bot

Releasing 0.23+14.10.20140521-0ubuntu1

540. By Olivier Tilloy

Fix FTBFS with Qt 5.3. Fixes: 1321440

541. By PS Jenkins bot

Releasing 0.23+14.10.20140521.1-0ubuntu1

542. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

543. By Olivier Tilloy

Various optimizations to the activity view. Fixes: 1260980

544. By Olivier Tilloy

Do not override the default height of the TextField that serves as the address bar. Fixes: 1317866

545. By PS Jenkins bot

Releasing 0.23+14.10.20140522.1-0ubuntu1

546. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

547. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

548. By Olivier Tilloy

Ensure the current webview is hidden while the activity view is visible,
and work around a bug in oxide that prevented new empty tabs from rendering.

549. By PS Jenkins bot

Releasing 0.23+14.10.20140526-0ubuntu1

550. By Olivier Tilloy

Ensure the main page that contains the webviews fills the main view. Fixes: 1321462

551. By PS Jenkins bot

Releasing 0.23+14.10.20140526.1-0ubuntu1

552. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

553. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

554. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

555. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

Revision history for this message
Olivier Tilloy (osomon) wrote :

There’s a bunch of trivial conflicts when merging this branch into the latest trunk, can you please fix them?

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

done

Revision history for this message
Olivier Tilloy (osomon) wrote :

I have a bunch of comments, see inline.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:556
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/846/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/557/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic-touch/141/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-amd64-ci/45
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-i386-ci/45
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/498/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/696
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/696/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1166
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1166/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/6776/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7981

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/846/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:558
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/849/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic/562/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-utopic-touch/144/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-amd64-ci/48
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/48
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-armhf-ci/48/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-utopic-i386-ci/48
    FAILURE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-utopic/502/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/701
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-amd64/701/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1180
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1180/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/6780/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7993

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/849/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

updated

Revision history for this message
Olivier Tilloy (osomon) wrote :

5 more comments inline.

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

To comment on a few notes:

> The type of the parameter for all those three signals should be 'url', not 'string'.

As I said earlier: yes I agree, but I get dbus runtime error on the AP side while trying to marshal the 'url' qml type to the QUrl qt type (unknown to dbus), ...

> this looks unsafe and abuse-prone to me, can’t we devise a more secure way of enabling tests, that doesn’t involve opening a backdoor in the container?

It is tricky to do something clean w/ this. Either way the container needs to run with some context, I did a few updates that makes it a bit cleaner ...

Revision history for this message
Olivier Tilloy (osomon) wrote :

> > The type of the parameter for all those three signals should be 'url', not
> 'string'.
>
> As I said earlier: yes I agree, but I get dbus runtime error on the AP side
> while trying to marshal the 'url' qml type to the QUrl qt type (unknown to
> dbus), ...

Ok, that makes sense.

> > this looks unsafe and abuse-prone to me, can’t we devise a more secure way
> of enabling tests, that doesn’t involve opening a backdoor in the container?
>
> It is tricky to do something clean w/ this. Either way the container needs to
> run with some context, I did a few updates that makes it a bit cleaner ...

I like the environment variable approach better. Can we not get rid of the --do-not-filter-url-patterns command line switch and rely only on the presence of the env var? (btw I would also check the value of the env var, if it’s "0" or "false" it should be ignored)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
556. By Alberto Mardegan

Add online account support in the webapp container.

A new "accountProvider" command line parameter has been added to allow one to specify an account provider for the specific webapp launch. When used, one can specify a given provider, e.g. "facebook", to pull existing accounts (if any) from the specified provider
from Online Accounts.

When the provider does not have any existing account, an option to delegate the creation of such an account to OA is provided. It is also possible to skip the step and go straight to the target url.

557. By PS Jenkins bot

Releasing 0.23+14.10.20140602-0ubuntu1

558. By Alberto Mardegan

Fix the cookies unit tests

Make sure that the dbPath always refers to a file, not a directory.
Make sure that even on fast machines (or machines with a low-accuracy timer) the record timestamps are unique.

559. By Olivier Tilloy

Add a UA override rule for m.youtube.com to allow playing videos. Fixes: 1228415

560. By Olivier Tilloy

Do not display an empty contextual menu. Fixes: 1326752

561. By Michael Sheldon

Filter file uploads based on mime-type information (where available)

562. By PS Jenkins bot

Releasing 0.23+14.10.20140605-0ubuntu1

563. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

564. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

565. By Olivier Tilloy

Move version 0.2 of the API to "Ubuntu.Web" namespace, rename "UbuntuWebView" to "WebView", and document the public API intended for application developers.

The legacy namespace and component name are kept around for compatibility with existing applications.

Version 0.1 of the API remains untouched (it is deprecated).

Packaging changes:
 - new qtdeclarative5-ubuntu-web-plugin package that contains the new namespace and name for the public QML API
 - updated runtime dependencies for webbrowser-app, webapp-container and qtdeclarative5-ubuntu-ui-extras-browser-plugin
 - updated descriptions for all packages
 - added missing multiarch stanza for webapp-container-autopilot
 - added missing predepends stanzas for multiarch packages Fixes: 1324180

566. By PS Jenkins bot

Releasing 0.23+14.10.20140609-0ubuntu1

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

updated

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
567. By Olivier Tilloy

Use encodeURIComponent() to encode search queries entered in the address bar. Fixes: 1314673

568. By PS Jenkins bot

Releasing 0.23+14.10.20140609.1-0ubuntu1

569. By David Barth

Accept navigation inside popups if the redirection stays within the set of accepted URLs. This helps support the account switching feature in Gmail for example.

An additional heuristics ensures that trampoline URLs don't leave a blank window in their originating webapp, as we navigate back to the page just before the trampoline. This avoids regressions in Facebook, Twitter and other apps using popup redirects to open external links. Fixes: 1324848

570. By PS Jenkins bot

Releasing 0.23+14.10.20140612.1-0ubuntu1

571. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

572. By Olivier Tilloy

debian/control: add fonts-liberation as a runtime dependency of webbrowser-app and webapp-container, for smoother rendering of webpages on devices. Fixes: 1322456

573. By Ugo Riboni

Update the application icon to the new suru theme.

This applies to desktop only. The current mobile theme (ubuntu-mobile-icons) still ships the old icon. Fixes: 1328147

574. By PS Jenkins bot

Releasing 0.23+14.10.20140617-0ubuntu1

575. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

576. By Olivier Tilloy

Honour fullscreen requests. Fixes: 1308947, 1328168

577. By PS Jenkins bot

Releasing 0.23+14.10.20140618.2-0ubuntu1

578. By PS Jenkins bot

No change rebuild against Qt 5.3

579. By PS Jenkins bot

Releasing 0.23+14.10.20140618.3-0ubuntu1

580. By Olivier Tilloy

Re-enable the geolocation permission request dialog, now that it is implemented in oxide.
Fix it in the webkit-based webapp container. Fixes: 1182658

581. By Olivier Tilloy

Temporarily work around bug #1328839 in qtubuntu by not toggling fullscreen on the window on devices.

Once the Qt compositor work lands (scheduled for mid July), the bug should be resolved and this workaround can be removed. Fixes: 1328839

582. By PS Jenkins bot

Releasing 0.23+14.10.20140620-0ubuntu1

583. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

584. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

585. By Michael Sheldon

Add support for sharing links via content-hub. Fixes: 1294764

586. By PS Jenkins bot

Releasing 0.23+14.10.20140627-0ubuntu1

587. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

588. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

589. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

590. By Olivier Tilloy

Re-enable contextual selection that had been disabled when switching to oxide.

Packaging change: renamed the qtdeclarative5-ubuntu-ui-extras-browser-plugin-assets package to qtdeclarative5-ubuntu-web-plugin-assets. Fixes: 1324292

591. By PS Jenkins bot

Releasing 0.23+14.10.20140630-0ubuntu1

592. By Olivier Tilloy

Add a user script that prevents so-called "smart banners" generated by the smartbanner jQuery plugin to show ads for native android/iOS apps. Fixes: 1329799

593. By Olivier Tilloy

Add support for custom search engines defined by the OpenSearch description document format
(http://www.opensearch.org/Specifications/OpenSearch/1.1). Fixes: 1277637, 1334546

594. By PS Jenkins bot

Releasing 0.23+14.10.20140630.1-0ubuntu1

595. By PS Jenkins bot

Resync trunk

596. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

597. By Launchpad Translations on behalf of phablet-team

Launchpad automatic translations update.

598. By Alexandre Abreu

merge lp:~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
599. By Alexandre Abreu

fix

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

updated

Revision history for this message
Olivier Tilloy (osomon) wrote :

In a previous comment, I was suggesting to remove the "--block-open-external-urls" command-line switch, but I’m seeing what I assume is a leftover in src/app/webcontainer/webapp-container.cpp.

Revision history for this message
Olivier Tilloy (osomon) wrote :

$ pep8 tests
tests/autopilot/webapp_container/tests/__init__.py:56:80: E501 line too long (92 > 79 characters)
tests/autopilot/webapp_container/tests/__init__.py:75:80: E501 line too long (82 > 79 characters)

Can you please fix those two pep8 errors?

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> In a previous comment, I was suggesting to remove the "--block-open-external-
> urls" command-line switch, but I’m seeing what I assume is a leftover in
> src/app/webcontainer/webapp-container.cpp.

I think your previous comment (that was addressed) was more about the "doFilterWebappUrlPatterns" flag which was now updated & cleaned w/ a env var approach,

I couldn't find references of a comment about the "--block" flag,

600. By Alexandre Abreu

pep8 fixes

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> $ pep8 tests
> tests/autopilot/webapp_container/tests/__init__.py:56:80: E501 line too long
> (92 > 79 characters)
> tests/autopilot/webapp_container/tests/__init__.py:75:80: E501 line too long
> (82 > 79 characters)
>
>
> Can you please fix those two pep8 errors?

done

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

425 +from __future__ import absolute_import
522 +from __future__ import absolute_import

Those should probably be removed.

Revision history for this message
Olivier Tilloy (osomon) wrote :

445 + self.assertThat(lambda: webview.url,
446 + Eventually(Contains("/href-link")))

In case like this where comparing the value of a property on an object, the use of a lambda is superfluous, i.e. this will work:

    self.assertThat(webview.url, Eventually(Contains("/href-link")))

There are a number of places in the new tests that can be simplified.

Revision history for this message
Olivier Tilloy (osomon) wrote :

To get back (again, sorry…) on the WEBAPP_CONTAINER_AP_TESTS_NOT_FILTER_URL_PATTERNS environment variable mechanism, if I understand correctly you added this "backdoor" because UrlPatternUtils::transformWebappSearchPatternToSafePattern(…) doesn’t recognize patterns that contain a port number, right?

If this is correct, how hard (and desirable) would it be to update the method to recognize patterns that contain a port number? It sounds like this could be useful, as the port number is part of the URL specification, so there could be webapps with a well-known domain name that require it, e.g. https://mycoolwebapp.com:12345/.

Revision history for this message
Olivier Tilloy (osomon) wrote :

545 + self.assertThat(lambda: popup_watcher.was_emitted,
546 + Eventually(Equals(popup_should_be_opened)))

In the case where popup_should_be_opened is False (i.e. on devices), I don’t think this test is useful, because popup_watcher.was_emitted is initially False anyway, before it changes to True.
So you probably want to make this assertion conditional instead:

    if popup_should_be_opened:
        self.assertThat(popup_watcher.was_emitted, Eventually(Equals(True)))

Revision history for this message
Olivier Tilloy (osomon) wrote :

547 + self.assertThat(lambda: url_watcher.was_emitted,
548 + Eventually(Equals(False)))

The same remark applies to this kind of assertion: url_watcher.was_emitted is initially False, and the use of Eventually() doesn’t mean that autopilot will wait for some time before initially testing the assertion. So you might as well change it to:

    self.assertThat(url_watcher.was_emitted, Equals(False))

But this is racy at best. If you move it under the condition as suggested in the comment above, then it makes sense, because you check it after verifying that popup_watcher.was_emitted is True:

    if popup_should_be_opened:
        self.assertThat(popup_watcher.was_emitted, Eventually(Equals(True)))
        self.assertThat(url_watcher.was_emitted, Equals(False))

Revision history for this message
system-apps-ci-bot (system-apps-ci-bot) wrote :
review: Needs Fixing (continuous-integration)

Unmerged revisions

600. By Alexandre Abreu

pep8 fixes

599. By Alexandre Abreu

fix

598. By Alexandre Abreu

merge lp:~abreu-alexandre/webbrowser-app/add-ap-tests-for-navigation-and-popup

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

to status/vote changes: