[Merge] lp:~ahayzen/webbrowser-app/add-autopilot-multi-window-tests-001 into lp:webbrowser-app/staging

Olivier Tilloy olivier.tilloy at canonical.com
Thu Nov 3 11:04:52 UTC 2016


Review: Needs Fixing



Diff comments:

> 
> === modified file 'tests/autopilot/webbrowser_app/tests/__init__.py'
> --- tests/autopilot/webbrowser_app/tests/__init__.py	2016-09-20 21:56:25 +0000
> +++ tests/autopilot/webbrowser_app/tests/__init__.py	2016-10-31 13:31:58 +0000
> @@ -256,6 +262,34 @@
>                          os.kill(child.pid, signal)
>                          break
>  
> +    def switch_to_unfocused_window(self, target_window,
> +                                   expected_number_unfocused_windows=1):
> +        try:
> +            windows = [
> +                window for window in self.process_manager.get_open_windows()
> +                if window.application.desktop_file ==
> +                "webbrowser-app.desktop" and
> +                not window.is_focused
> +            ]
> +
> +            # There should be 1 unfocused window

The comment is incorrect, as the number of unfocused windows is a parameter of the function. Besides, it’s not really useful as the assertion is quite self-explanatory.

> +            self.assertThat(len(windows),
> +                            Equals(expected_number_unfocused_windows))
> +
> +            # Cycle through possible windows until target gets focus
> +            for window in windows:
> +                window.set_focus()
> +                self.assertThat(lambda: window.is_focused,
> +                                Eventually(Equals(True)))
> +
> +                if target_window.activeFocus:
> +                    break
> +        except (RuntimeError, ImportError) as e:

flake8 error here (local variable 'e' is assigned to but never used)

> +            # Fallback to clicking on the window
> +            self.pointing_device.click_object(target_window)
> +
> +        self.assertThat(target_window.activeFocus, Eventually(Equals(True)))
> +
>  
>  class StartOpenRemotePageTestCaseBase(BrowserTestCaseBase):
>  
> 
> === modified file 'tests/autopilot/webbrowser_app/tests/test_site_previews.py'
> --- tests/autopilot/webbrowser_app/tests/test_site_previews.py	2016-02-15 13:30:32 +0000
> +++ tests/autopilot/webbrowser_app/tests/test_site_previews.py	2016-10-31 13:31:58 +0000
> @@ -168,3 +168,11 @@
>          self.remove_top_site(new_tab_view, previous)
>          self.assertThat(lambda: self.file_in_dir(capture, self.captures_dir),
>                          Eventually(Equals(False)))
> +
> +    def test_private_window_uses_private_tab_view(self):

This test doesn’t belong here, that file tests only the site previews (as images written to disk in the cache folder).
It would be better suited in test_new_tab_view.py.

> +        self.open_new_private_window()
> +
> +        private_window = self.app.get_windows(incognito=True)[0]
> +
> +        self.assertThat(private_window.get_new_private_tab_view().visible,
> +                        Eventually(Equals(True)))


-- 
https://code.launchpad.net/~ahayzen/webbrowser-app/add-autopilot-multi-window-tests-001/+merge/309678
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app/staging.



More information about the Ubuntu-reviews mailing list