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

Only use Sizzle when required #18896

Merged
merged 2 commits into from
Jul 31, 2023
Merged

Conversation

jelly
Copy link
Member

@jelly jelly commented Jun 7, 2023

This was a test PR, this would be a step towards doing our own :contains implementation. First get all our projects working correctly with document.querySelector and then in the future see if we can do a :contains implementation.


Just a test PR, to see if we really only need sizzle for :contains.

Failing tests on fedora-coreos, blocked on patternfly/patternfly-react#9399

  • TestTimers.testCreate
  • TestLogin.testBypassSoftRequirements
  • TestMetricsPackages.testBasic

@jelly
Copy link
Member Author

jelly commented Jun 7, 2023

  File "/work/bots/make-checkout-workdir/test/verify/check-networkmanager-firewall", line 117, in testNetworkingPage
    b.click(".pf-v5-c-breadcrumb li:first")
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 387, in click
    self.mouse(selector + ":not([disabled]):not([aria-disabled=true])", "click", 0, 0, 0)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 294, in raise_cdp_exception
    raise Error("%s(%s): %s" % (func, arg, msg))
testlib.Error: timeout
wait_js_cond(ph_is_present(".pf-v5-c-breadcrumb li:first:not([disabled]):not([aria-disabled=true])")): Uncaught (in promise)
cdp: {'source': 'network', 'level': 'error', 'text': 'Failed to load resource: the server responded with a status of 401 (Authentication failed)', 'timestamp': 1686141202260.862, 'url': 'http://127.0.0.2:9191/cockpit/login', 'networkRequestId': '22105.12'}

:first is not a valid css pseudo selector so we also have to get rid of that.

A bunch of storage related tests fail like this:

Traceback (most recent call last):
  File "/work/bots/make-checkout-workdir/test/verify/check-storage-mounting", line 157, in testMountingHelp
    self.content_tab_wait_in_info(1, 1, "Name", "FILESYSTEM")
  File "/work/bots/make-checkout-workdir/test/common/storagelib.py", line 224, in content_tab_wait_in_info
    self.retry(setup, check, teardown)
  File "/work/bots/make-checkout-workdir/test/common/storagelib.py", line 46, in retry
    self.browser.wait(step)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 574, in wait
    raise Error('timed out waiting for predicate to become true')

Not specifying a html element probably does not work with querySelector

> warning: transport closed: disconnected
Traceback (most recent call last):
  File "/work/bots/make-checkout-workdir/test/verify/check-system-services", line 1336, in testCreate
    b.click("[data-index=0] [aria-label=Add]")
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 387, in click
    self.mouse(selector + ":not([disabled]):not([aria-disabled=true])", "click", 0, 0, 0)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 379, in mouse
    self.wait_visible(selector)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 617, in wait_visible

A few tests have weird issues with selecting an id requires some debugging:

Traceback (most recent call last):
  File "/work/bots/make-checkout-workdir/test/verify/check-metrics", line 1467, in testBasic
    b.wait_visible("#pcp-settings-modal")
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 294, in raise_cdp_exception
    raise Error("%s(%s): %s" % (func, arg, msg))
testlib.Error: timeout
wait_js_cond(ph_is_visible("#pcp-settings-modal")): Uncaught (in promise) Error: #pcp-settings-modal is ambiguous

@jelly jelly closed this Jul 10, 2023
@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 24, 2023
@jelly jelly reopened this Jul 24, 2023
@jelly
Copy link
Member Author

jelly commented Jul 24, 2023

Issues, seem to boil down to:

document.querySelectorAll("#pcp-settings-modal")
NodeList(2) [div#pcp-settings-modal.pf-v5-c-modal-box.pf-m-align-top.pf-m-sm, div#pcp-settings-modal.pf-v5-c-modal-box__body]0: div#pcp-settings-modal.pf-v5-c-modal-box.pf-m-align-top.pf-m-sm1: div#pcp-settings-modal.pf-v5-c-modal-box__bodylength: 2[[Prototype]]: NodeList

Basically blocked on patternfly/patternfly-react#9399

@jelly jelly added the blocked Don't land until something else happens first (see task list) label Jul 24, 2023
@jelly jelly force-pushed the sizzle-test branch 3 times, most recently from 46d4a90 to 12b411a Compare July 24, 2023 13:38
@jelly jelly removed blocked Don't land until something else happens first (see task list) no-test For doc/workflow changes, or experiments which don't need a full CI run, labels Jul 31, 2023
Due to a PatternFly bug PCP's settings modal has two identical id's
`pcp-settings-modal`. Sizzle was very liberal and selected the first id
it would find, however using `document.querySelector` this fails as the
result is ambiguous. Instead of replicating the wrong behaviour of
allowing duplicate id's make this fail `ph_select` and work around the
test until PF fixed the modal component.
@jelly jelly requested review from martinpitt and marusak July 31, 2023 12:29
@jelly jelly marked this pull request as ready for review July 31, 2023 12:30
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks for the extra effort of fixing that pcp dialog! I like this a lot -- it confirms that we at least don't use any magic selectors aside from :contains(), and we now have the option of running without sizzle in smaller projects. Yay!

@martinpitt martinpitt merged commit ff6f5a8 into cockpit-project:main Jul 31, 2023
30 checks passed
@jelly jelly deleted the sizzle-test branch July 31, 2023 15:23
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.

2 participants