Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Move from CDP to BiDi #401

Merged
merged 4 commits into from
Aug 15, 2024
Merged

Conversation

KKoukiou
Copy link
Contributor

See cockpit-project/cockpit#20832

Drop chrome-remote-interface NPM dependency.

Explicitly add a "glob" devDependency to follow suit with cockpit-project/cockpit@680decc155a
It was previously used implicitly through a transient dependency of something else, but our esbuild po-plugin uses it explicitly.

Copy link
Contributor Author

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinpitt i have two tests that check exception handling fail with this bidi convertion.

both do something like: 253 b.eval_js("window.setTimeout(function() { myNonExistingFunction()}, 0);")

And because the UI handles unhandled JS errors with window.onerror I expect to see the error dialog.

With bidi this does not propagate properly. Any ideas what's missing?

Otherwise I will disable the two tests.

@martinpitt
Copy link
Collaborator

Some notes:

  • The old CDP API didn't have reliable exception reporting, it was async and delayed. That was the reason why you had to do these loops:
  b.eval_js("window.setTimeout(function() { myNonExistingFunction()}, 0);"
  # Some round trips, one of which should update the deferred exception
  for _ in range(0, 5):
      b.wait_in_text("#critical-error-bz-report-modal-details", "myNonExistingFunction is not defined")
      time.sleep(2)

With BiDi, this will happen immediately and reliably if the command or returned promise rejects. However, the called command does not fail, as it just starts a timer and does not return a promise. So you can drop the loop (as it's pointless), and drop the with self.assertRaises(RuntimeError): as the eval_js() itself will reliably not fail. You only get the console error:

error: ReferenceError: myNonExistingFunction is not defined

from the timer when it fires, but as nothing pays attention to that, this doesn't cause any crash.

  • Calling the timeout crash fn did not trigger Cockpit's general "Oops" handler with CDP either. This happens in CDP context, not page context. Supposedly Anaconda's error handler uses a different approach than cockpit's Oops handler.

We test that in cockpit in https://github.com/cockpit-project/cockpit/blob/main/pkg/playground/exception.js , and the test clicks that button to provoke a crash. Taking that idea, I was trying to do this not with a timer, but with setting an event handler inside of the actual page:

b.eval_js("window.document.getElementById('toolbar').addEventListener('click', () => myNonExistingFunction())")
b.click("#toolbar")

Doing that in the JS console/inspector and clicking the toolbar works fine, but not through eval_js. Apparently this is running in a "shielded" context.

@KKoukiou Sorry, I'm afraid this is a hard problem. My suggestion would be to add that crasher into the actual page somehwere, in a hidden place (making it conditional on running as test or so?), and then activate it with a click.

@KKoukiou
Copy link
Contributor Author

@martinpitt I just realized that the test correctly triggers the exception. I see the exception handling UI in the screenshots. The problem is that RuntimeError was never raised, aka - removing with self.assertRaises(RuntimeError): fixes the issue. Thanks for taking a look.

See cockpit-project/cockpit#20832

Drop chrome-remote-interface NPM dependency.

Explicitly add a "glob" devDependency to follow suit with
cockpit-project/cockpit@680decc155a
It was previously used implicitly through a transient dependency of
something else, but our esbuild po-plugin uses it explicitly.

Adjust the exception handling tests to not wait for RuntimeError, this
is no longer raised. Also switch_to_top frame when waiting for the error
dialog (no idea how it worked before).
Lastly make the CockpitJSErrorHandling test nondestructive.
Now that all tests contained are non destructive.
Copy link
Contributor

@marusak marusak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@KKoukiou KKoukiou removed the request for review from martinpitt August 15, 2024 11:08
Copy link
Contributor

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. :)

@KKoukiou KKoukiou merged commit 301f2d0 into rhinstaller:main Aug 15, 2024
7 checks passed
@KKoukiou KKoukiou deleted the bidi-convert branch August 15, 2024 11:23
@martinpitt
Copy link
Collaborator

@KKoukiou this looks great now, thanks! I'm glad it was something easy after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants