test: Implement Firefox testing with CDP #12971

marusak · 2019-10-14T05:59:28Z

Firefox CDP is only partially implemented and some major features are missing. Also chrome-remote-interface misses some features on Firefox. Therefore I've created new cdp-driver which contains some hacks and workarounds. Since it uses very small subset of cdp protocol, it could be used for chrome as well, but lets keep also the proper driver and hopefully one day we can use that one for all browsers.

How to run this: (see note below)
prerequisite: Firefox browser (tested with current F31 Firefox build 69.0.1-3)

TEST_BROWSER=firefox ./test/verify/check-pages TestPages.testBasic -vt

TODO:

Implement screenshots
opening new tabs in Firefox (not needed for CI or for TEST_WITH_UI, but when connecting to running browser)
testBasic (check_dashboard.TestBasicDashboard)
testNoConnectionSettings (check_networking_settings.TestNetworking)

1 2

marusak · 2019-11-01T12:02:09Z

New Firefox (firefox-70.0-1.fc31.x86_64 and current nightly) breaks propagation of new contexts. I cannot think of any workaround for this since this is a core functionality that we need. I reported it here. I can either enforce firefox <= 69 in CI or block on that.

marusak · 2019-11-08T07:33:28Z

uuu, fancy! Only one failure, but it is currently broken testLibvirt one and one test was retried. I think this is as good as it gets :)

martinpitt · 2019-11-08T09:24:14Z

#13113 landed, this needs a rebase now.

martinpitt

Thanks for this amazing work! As usual, I have a lot of smaller nitpicks, and I'd like to know the next steps for firefox-cdp-driver.js. But let's get this in soon, so that we can split out the work of enabling the remaining tests between multiple persons.

martinpitt · 2019-11-11T06:57:16Z

test/common/cdp.py

@@ -52,6 +70,8 @@ def __init__(self, lang=None, verbose=False, trace=False, inject_helpers=[]):
        self.verbose = verbose
        self.trace = trace
        self.inject_helpers = inject_helpers
+        self.browser = "chromium"


I would find it nicer to correctly initialize it right here. Something like os.environ.get("TEST_BROWSER", "chromium")

martinpitt · 2019-11-11T06:58:37Z

test/common/cdp.py

@@ -74,7 +94,9 @@ def invoke(self, fn, **kwargs):
        # frame support for Runtime.evaluate(): map frame name to
        # executionContextId and insert into argument object; this must not be quoted
        # see "Frame tracking" in cdp-driver.js for how this works
-        if fn == 'Runtime.evaluate' and self.cur_frame:
+        # Also Firefox always needs context, cannot work with default context.
+        # TODO: it should be fairly simple for chromium to always work with contexts


Did you just not try yet, or does this actively break right now?

I just did small edit in chrome driver and now this was dropped

martinpitt · 2019-11-11T06:59:46Z

test/common/cdp.py

@@ -155,6 +212,9 @@ def start(self):
                # can fail when a test starts multiple browsers; only show the first one
                cdp_port = p

+        if "TEST_BROWSER" in os.environ:
+            self.browser = os.environ["TEST_BROWSER"]


Let's drop this, see above.

martinpitt · 2019-11-11T07:04:29Z

test/common/cdp.py

+        elif self.browser == "firefox":
+            subprocess.Popen(["firefox", "--headless", "--no-remote", "-CreateProfile", "blank"], env=env).communicate()
+            p = subprocess.Popen("ls ~/.mozilla/firefox", shell=True, stdout=subprocess.PIPE, env=env)
+            in_profile = p.communicate()[0].decode("utf-8").split("\n")


Please use os.listdir() here. Also, writing ~ here makes me terribly nervous -- we set $HOME here, but let's not use os.path.expanduser() here or ~, but explicitly sef._browser_home to make sure that we don't write into the real user home.

martinpitt · 2019-11-11T07:06:05Z

test/common/cdp.py

+                if item.endswith("blank"):
+                    handler = os.path.join(handler, item, "handlers.json")
+                    userjs = os.path.join(userjs, item, "user.js")
+                    break


Looks like you don't really need to read the whole directory in the first place? How is that different to simply doing os.path.join(sef._browser_home, "blank")? (And then tacking on the two *.json, of course).

The path looks something like ~/.mozilla/firefox/ldsyl7au.blank/. I could use glob though?

Argh, sorry -- I misread the code. Yes, glob seems fine indeed.

test/common/testlib.py

martinpitt · 2019-11-11T07:25:15Z

test/verify/check-sosreport

-        download_dir = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, download_dir)
-        b.cdp.invoke("Page.setDownloadBehavior", behavior="allow", downloadPath=download_dir)
+        for f in glob.glob(os.path.join(b.cdp.download_dir, '*')):


OK, but more robust to do that with os.listdir(). But why does that need to be cleaned in the first place? Every test case starts with a fresh Browser and thus CDP object, and thus a new empty download dir?

Right, this was thinko since it is created in CDP init and this it is clean for every test.

martinpitt · 2019-11-11T07:29:15Z

test/verify/check-menu

+        # HACK: We cannot test axe on Firefox since `axe.run()` returns promise
+        # and Firefox CDP cannot wait for it to resolve
+        if b.cdp.browser != "firefox":
+            self.check_axe("Test-navigation")


Please put this hack into the check_axe() definition itself, instead of into every call (well, two right now, but we'll get more)

test/common/firefox-cdp-driver.js

martinpitt

Nice, thanks! Just a nitpick, but not crucial to force-push just for that. Let's see what the tests say.

test/common/cdp.py

Closes cockpit-project#12971

marusak · 2019-11-12T05:54:16Z

@martinpitt I had to repush as after rebase one test needed a small tweak. Can you please reapprove?

martinpitt · 2019-11-12T06:30:50Z

testPrivateKeys fails an awful lot here. That doesn't ring a bell, I rather retry that one.

marusak · 2019-11-12T07:06:59Z

testPrivateKeys again failed on ubuntu-1804 but not anywhere else.
But it also happens in different PRs like here.
We don't need to block this on it. I'll pull this image and debug it locally.

marusak · 2019-11-12T07:31:54Z

Fixed in #13131

marusak added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 14, 2019

marusak force-pushed the firefox_cdp branch from d7d1fc9 to ccb5cf8 Compare October 17, 2019 20:48

marusak mentioned this pull request Oct 18, 2019

test: Obsolete sizzle #13015

Closed

marusak force-pushed the firefox_cdp branch 2 times, most recently from 96a5531 to 64b2722 Compare October 21, 2019 09:19

marusak changed the title ~~POC: WIP: Run verify tests in Firefox~~ WIP: Run verify tests in Firefox Oct 21, 2019

marusak force-pushed the firefox_cdp branch 3 times, most recently from 1ebbbc9 to d1f1fd3 Compare October 23, 2019 09:29

marusak force-pushed the firefox_cdp branch 2 times, most recently from b0973c9 to 295c0d2 Compare October 31, 2019 12:19

marusak force-pushed the firefox_cdp branch 4 times, most recently from 25d5c46 to 05460a1 Compare November 8, 2019 06:39

marusak requested a review from martinpitt November 8, 2019 07:33

marusak mentioned this pull request Nov 8, 2019

Some tests mods to prepare for Firefox testing #13113

Merged

martinpitt added the needs-rebase label Nov 8, 2019

marusak force-pushed the firefox_cdp branch from 05460a1 to 053f87e Compare November 9, 2019 17:22

marusak removed the needs-rebase label Nov 9, 2019

martinpitt requested changes Nov 11, 2019

View reviewed changes

marusak removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 11, 2019

marusak force-pushed the firefox_cdp branch 2 times, most recently from fa7ef06 to 7ba5f00 Compare November 11, 2019 13:45

martinpitt previously approved these changes Nov 11, 2019

View reviewed changes

test/common/cdp.py Outdated Show resolved Hide resolved

marusak dismissed martinpitt’s stale review via 8801f8a November 11, 2019 17:06

marusak force-pushed the firefox_cdp branch from 7ba5f00 to 8801f8a Compare November 11, 2019 17:06

test: Implement Firefox testing with CDP

5ac0eb7

Closes cockpit-project#12971

marusak force-pushed the firefox_cdp branch from 8801f8a to 5ac0eb7 Compare November 11, 2019 17:42

marusak requested a review from martinpitt November 12, 2019 05:53

martinpitt approved these changes Nov 12, 2019

View reviewed changes

martinpitt changed the title ~~WIP: Run verify tests in Firefox~~ test: Implement Firefox testing with CDP Nov 12, 2019

martinpitt merged commit 9ebd346 into cockpit-project:master Nov 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Implement Firefox testing with CDP #12971

test: Implement Firefox testing with CDP #12971

marusak commented Oct 14, 2019 •

edited

marusak commented Nov 1, 2019

marusak commented Nov 8, 2019

martinpitt commented Nov 8, 2019

martinpitt left a comment

martinpitt Nov 11, 2019

martinpitt Nov 11, 2019

marusak Nov 11, 2019

martinpitt Nov 11, 2019

martinpitt Nov 11, 2019

martinpitt Nov 11, 2019

marusak Nov 11, 2019

martinpitt Nov 11, 2019

martinpitt Nov 11, 2019

marusak Nov 11, 2019

martinpitt Nov 11, 2019

martinpitt left a comment

marusak commented Nov 12, 2019

martinpitt commented Nov 12, 2019

marusak commented Nov 12, 2019

marusak commented Nov 12, 2019

test: Implement Firefox testing with CDP #12971

test: Implement Firefox testing with CDP #12971

Conversation

marusak commented Oct 14, 2019 • edited

marusak commented Nov 1, 2019

marusak commented Nov 8, 2019

martinpitt commented Nov 8, 2019

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

marusak commented Nov 12, 2019

martinpitt commented Nov 12, 2019

marusak commented Nov 12, 2019

marusak commented Nov 12, 2019

marusak commented Oct 14, 2019 •

edited