Implement Nightwatch tests for Navigation module

Created on 11 October 2023, about 1 year ago
Updated 6 July 2024, 6 months ago

Problem/Motivation

By @finnsky in 🌱 [PLAN] Accessibility review Active :

We could add Nightwatch tests because we have automated Nightwatch a11y tests in core: πŸ“Œ Automated A11y tests in Nightwatch Fixed . Added simple tests using it.

How to start with Nightwatch testing can be found in `core/tests/README.md` file from Drupal core.

This issue should be focused on implementing a first simple test, like "(If logo) it takes you to the homepage" and define the structure the rest of the tests will follow.

πŸ“Œ Task
Status

Fixed

Version

10.4 ✨

Component
NavigationΒ  β†’

Last updated about 20 hours ago

No maintainer
Created by

πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @ckrina
  • First commit to issue fork.
  • πŸ‡·πŸ‡ΈSerbia finnsky

    We also need:

    1. Own testing tag EG: '@tags': ['navigation'],
    2. Add tests in different themes contexts. Umami / Claro / Olivero / NO_THEME

    Like was done https://git.drupalcode.org/project/navigation/-/merge_requests/79/diffs

  • Status changed to Needs review about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

    The nightwatch MR has basic accessibility tests running. It's good to have these running, and this demonstrates that adding Nightwatch doesn't require any major errfor.

    The Axe A11y tests already caught a duplicate ID, which is now fixed in that same MR. Given that this already caught an accessibility bug, I think this basic implementation should get in sooner than later so it can surface other accessibility problems as this project evolves.

    The NW testing can get more comprehensive than what is currently there if we see the need. I think beginning with something simple like this is helpful as it gets basic A11y checks going and is a starting point for additional NW tests.

  • Merge request !133Nightwatch 3393400 β†’ (Open) created by bnjmnm
  • πŸ‡·πŸ‡ΈSerbia finnsky

    Also creating some `dark` testing theme could be useful here.
    Like:
    body { background-color: black; color: white; }

  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    I wonder if πŸ“Œ Toolbar doesn't indicate active menu trail for pages not included in Toolbar Active should be a classic test or a Nightwatch one.

  • Merge request !178Resolve #3393400 "Nightwatch tests" β†’ (Open) created by finnsky
  • πŸ‡·πŸ‡ΈSerbia finnsky

    finnsky β†’ changed the visibility of the branch nightwatch to hidden.

  • πŸ‡·πŸ‡ΈSerbia finnsky

    finnsky β†’ changed the visibility of the branch 3393400-evaluate-adding-nightwatch to hidden.

  • Pipeline finished with Canceled
    10 months ago
    Total: 144s
    #97479
  • Pipeline finished with Failed
    10 months ago
    Total: 192s
    #97480
  • πŸ‡·πŸ‡ΈSerbia finnsky

    Added a11y test and expand/collapse test.

    Ally reports now

       βœ– 1) Tests/a11y
    
       – Accessibility - Navigation Module - Claro page (10.3s)
    
       β†’ βœ– NightwatchAssertError
       aXe rule: button-name - Buttons must have discernible text
            In element: .toolbar-button--icon--sidebar-toggle
       β†’ βœ– NightwatchAssertError
       aXe rule: link-name - Links must have discernible text
            In element: .toolbar-button--icon--system-themes-page
       β†’ βœ– NightwatchAssertError
       aXe rule: link-name - Links must have discernible text
            In element: .toolbar-button--icon--system-modules-list
       β†’ βœ– NightwatchAssertError
       aXe rule: link-name - Links must have discernible text
            In element: .toolbar-button--icon--entity-user-collection
    
       – Accessibility - Navigation Module - HomePage (6.613s)
    
       β†’ βœ– NightwatchAssertError
       aXe rule: button-name - Buttons must have discernible text
            In element: .toolbar-button--icon--sidebar-toggle
       β†’ βœ– NightwatchAssertError
       aXe rule: heading-order - Heading levels should only increase by one
            In element: .label
       β†’ βœ– NightwatchAssertError
       aXe rule: landmark-unique - Ensures landmarks are unique
            In element: #admin-toolbar
       β†’ βœ– NightwatchAssertError
       aXe rule: link-name - Links must have discernible text
            In element: .toolbar-button--icon--system-themes-page
       β†’ βœ– NightwatchAssertError
       aXe rule: link-name - Links must have discernible text
            In element: .toolbar-button--icon--system-modules-list
       β†’ βœ– NightwatchAssertError
       aXe rule: link-name - Links must have discernible text
            In element: .toolbar-button--icon--entity-user-collection
       β†’ βœ– NightwatchAssertError
       aXe rule: region - All page content should be contained by landmarks
            In element: #primary-tabs-title
    

    We need to add tooltip tests and mobile. basically will be fine

  • Pipeline finished with Failed
    10 months ago
    #97488
  • πŸ‡·πŸ‡ΈSerbia finnsky

    hello all!
    I have small misunderstanding.
    We want to add Nightwatch testing. But i don’t really want to reproduce all Drupal Nightwatch infrastructure in module. Currently I’m running tests from core dir like yarn run test:nightwatch --tag navigation
    But it will be useless in Navigation module CI.

    So idk. I’ll continue in same approach. Probably we need to move Drupal Nightwatch functions into npm package. So it can be useful for modules/themes CI

    Idk seems CI runned somehow. Need to check its context.

  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100263
  • Pipeline finished with Success
    10 months ago
    #100255
  • Pipeline finished with Success
    10 months ago
    Total: 34s
    #100268
  • Pipeline finished with Success
    10 months ago
    Total: 63s
    #100318
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100338
  • Pipeline finished with Success
    10 months ago
    Total: 350s
    #100349
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100358
  • Pipeline finished with Failed
    10 months ago
    Total: 33s
    #100371
  • Pipeline finished with Failed
    10 months ago
    Total: 34s
    #100381
  • Pipeline finished with Success
    10 months ago
    Total: 34s
    #100392
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100422
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100429
  • Pipeline finished with Success
    10 months ago
    #100435
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100446
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100456
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100469
  • Pipeline finished with Success
    10 months ago
    Total: 34s
    #100482
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100490
  • Pipeline finished with Success
    10 months ago
    Total: 35s
    #100502
  • Pipeline finished with Success
    10 months ago
    #100528
  • Pipeline finished with Success
    10 months ago
    Total: 4072s
    #100556
  • Pipeline finished with Success
    10 months ago
    Total: 298s
    #100644
  • Pipeline finished with Success
    10 months ago
    Total: 93s
    #100703
  • Pipeline finished with Success
    10 months ago
    #100710
  • Pipeline finished with Success
    10 months ago
    #100712
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100732
  • Pipeline finished with Success
    10 months ago
    Total: 33s
    #100752
  • Pipeline finished with Success
    10 months ago
    Total: 60751s
    #100757
  • Pipeline finished with Success
    10 months ago
    Total: 167s
    #103656
  • Pipeline finished with Success
    10 months ago
    Total: 166s
    #103657
  • Status changed to Needs work 10 months ago
  • Pipeline finished with Success
    9 months ago
    Total: 199s
    #120553
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    Ideas on things that we could test:

    • Collapse/expand the sidebar
    • Scroll the sidebar (if the β€œcontent” is higher than the viewport height + footer)
    • Open/Close the drawer on hover (submenu level 2)
      • Test it with edge cases, like moving fast to the top of the drawer - so the mouse leaves the targeted hover item
    • Navigate to submenu level 3 (accordion)
    • Navigate to User menu links
    • See labels/tooltips of the 1st level links
    • Active menu item on 1st, 2nd and 3rd level items
    • (If logo) it takes you to the homepage
    • Active item shows as so in the Toolbar
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    We've discussed this on several occasions and we've agreed on the need to implement these tests. So changing this from "evaluate" to "implement".

  • Pipeline finished with Success
    9 months ago
    Total: 52s
    #138376
  • Pipeline finished with Success
    9 months ago
    Total: 317s
    #139940
  • Pipeline finished with Failed
    8 months ago
    #152542
  • Pipeline finished with Success
    8 months ago
    Total: 809s
    #152552
  • Pipeline finished with Success
    8 months ago
    Total: 325s
    #153270
  • Pipeline finished with Canceled
    8 months ago
    Total: 709s
    #156519
  • Pipeline finished with Success
    8 months ago
    Total: 1110s
    #156528
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona
  • Pipeline finished with Success
    8 months ago
    Total: 341s
    #164209
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona
  • First commit to issue fork.
  • Merge request !7980Implement Nightwatch tests β†’ (Closed) created by mglaman
  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    Started working on the tests, copied over @bnjmnm test https://git.drupalcode.org/project/drupal/-/merge_requests/7980

    Experimental modules can't be installed using the install module command. since it won't be experimental for long, I just used a test setup script

  • Pipeline finished with Failed
    8 months ago
    Total: 206s
    #167934
  • Pipeline finished with Failed
    8 months ago
    Total: 604s
    #168129
  • Pipeline finished with Failed
    8 months ago
    Total: 696s
    #168988
  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    We have a problem here. The scroll wrapper, .admin-toolbar__scroll-wrapper, causes WebDriver to think any elements within it are not visible for interaction. Oddly enough waitForElementVisible passes. Probably because the latter is inspecting the DOM and not interacting with it.

    When I made these changes, the tests passed:

    .admin-toolbar__scroll-wrapper {
      /*display: flex;*/
      /*overflow-x: hidden;*/
      /*overflow-y: auto;*/
      flex-direction: column;
    ...
    }
    
    @media (min-width: 64rem) {
      .admin-toolbar__scroll-wrapper {
        /*display: contents;*/
        /*background: none;*/
      }
    }
    

    I also needed to fix the footer's z-index in πŸ› Fix z-index Needs review . Once the wrapper adjustments were made, the footer was hidden behind the navigation.

  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    Okay, I worked backward and added back styles. It is the display: contents

    @media (min-width: 64rem) {
      .admin-toolbar__scroll-wrapper {
        display: contents;
    
  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    Buttons are not accessible with display: contents applied. See issues for Chromium, Firefox, and WebKit

  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    I guess this is blocked on πŸ› Buttons are not accessible with display: contents applied Active , then.

  • Pipeline finished with Running
    8 months ago
    #169048
  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    mglaman β†’ changed the visibility of the branch 3393400-nightwatch-tests to hidden.

  • Pipeline finished with Canceled
    8 months ago
    Total: 622s
    #169078
  • Pipeline finished with Failed
    8 months ago
    Total: 572s
    #169090
  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    The test is green, but:

    - cherry-pick fix from πŸ› Buttons are not accessible with display: contents applied Active
    - reverts πŸ› Fix z-index Needs review

  • Pipeline finished with Failed
    8 months ago
    Total: 4543s
    #169099
  • Pipeline finished with Failed
    8 months ago
    Total: 346s
    #169167
  • Pipeline finished with Running
    8 months ago
    #169178
  • Pipeline finished with Success
    8 months ago
    Total: 573s
    #169200
  • Status changed to Needs review 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA
  • Pipeline finished with Success
    8 months ago
    Total: 587s
    #169765
  • Pipeline finished with Canceled
    8 months ago
    Total: 156s
    #169772
  • Pipeline finished with Canceled
    8 months ago
    Total: 6s
    #169773
  • πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

    Also pushed the axe test. It is failing. I think we should:

    - Merge πŸ› Buttons are not accessible with display: contents applied Active to apply fix
    - Follow @ckrina idea of making this a meta issue
    - Commit the test site install script and expandCollapseTest.js (this issue or new child?)
    - Make the child issues from #14 including a11y test

  • Pipeline finished with Failed
    8 months ago
    Total: 664s
    #169776
  • Pipeline finished with Canceled
    7 months ago
    Total: 654s
    #170494
  • πŸ‡·πŸ‡ΈSerbia finnsky

    I've added special tag for module for :

    `yarn test:nightwatch --tag navigation`

  • Pipeline finished with Canceled
    7 months ago
    Total: 527s
    #170496
  • πŸ‡·πŸ‡ΈSerbia finnsky

    I've removed initial titles.
    Now they are not defined.
    Only block level titles we have now.

    Backender review required here.

    Test passed now.

  • πŸ‡·πŸ‡ΈSerbia finnsky

    I think we can merge it and create META for other tests.

  • Pipeline finished with Failed
    7 months ago
    Total: 620s
    #170501
  • Pipeline finished with Success
    7 months ago
    Total: 606s
    #170511
  • Status changed to Needs work 7 months ago
  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • πŸ‡«πŸ‡·France nod_ Lille
  • Status changed to Needs review 7 months ago
  • Pipeline finished with Success
    7 months ago
    Total: 538s
    #179017
  • Status changed to Needs work 7 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Left a comment on MR. Should be small fix.

  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    We just defined an strategy to keep titles for the menus and avoid the problems with cache.

  • First commit to issue fork.
  • Pipeline finished with Success
    7 months ago
    Total: 618s
    #179431
  • Status changed to Needs review 7 months ago
  • πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

    Updated the merge request to address the <h4> issue. See MR comment for details.

  • Status changed to Needs work 7 months ago
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    Added comment suggesting a different approach to set the <h4> title using the original logic we had in place.
    Let's decide the preferred approach and wrap this one!

  • Pipeline finished with Success
    7 months ago
    Total: 180s
    #180415
  • Pipeline finished with Success
    7 months ago
    Total: 239s
    #180418
  • Pipeline finished with Canceled
    7 months ago
    Total: 3006s
    #180505
  • Pipeline finished with Failed
    7 months ago
    Total: 628s
    #180523
  • Pipeline finished with Success
    7 months ago
    Total: 518s
    #181588
  • Pipeline finished with Success
    7 months ago
    Total: 552s
    #191169
  • Status changed to Needs review 7 months ago
  • πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

    Addressed comments from @plopesc. Ready for review.

  • Pipeline finished with Canceled
    7 months ago
    Total: 115s
    #191369
  • πŸ‡·πŸ‡ΈSerbia finnsky

    Looks good to me. Also i've removed outdated classnames. they are only should be on block level now.

  • Pipeline finished with Success
    7 months ago
    Total: 696s
    #191370
  • Status changed to Needs work 7 months ago
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    Thank you for the progress here!

    I'm missing these 2 small tasks from the strategy defined in #37

    • Remove ID from h4and aria-labelledby from ul
    • Open a follow-up to add back the ID and aria-labelled-by with JS to avoid caching issues. Also in this follow-up replace the ID on the li items, r evaluate if they are needed.

    Other than that, I think this is good to go!

  • Status changed to Needs review 7 months ago
  • πŸ‡·πŸ‡ΈSerbia finnsky

    Follow up https://www.drupal.org/project/drupal/issues/3452724 ✨ back the ID and aria-labelled-by with JS Postponed

  • Pipeline finished with Success
    7 months ago
    Total: 487s
    #191882
  • Status changed to RTBC 7 months ago
  • πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

    This is looking good to me! Items in #43 have been addressed. Tests are passing and making good sense to me.

  • πŸ‡·πŸ‡ΈSerbia finnsky

    Sorry, keep in in rtbc. we also had that heading ID in footer.

  • Pipeline finished with Success
    7 months ago
    Total: 604s
    #191948
  • Status changed to Needs work 7 months ago
  • πŸ‡«πŸ‡·France nod_ Lille

    few comments

  • Pipeline finished with Canceled
    6 months ago
    Total: 62s
    #200898
  • Pipeline finished with Success
    6 months ago
    Total: 498s
    #200899
  • Status changed to RTBC 6 months ago
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    These changes are not affecting the code and tests are still green, so I'm moving it back to RTBC directly.

    Thank you

  • Status changed to Needs work 6 months ago
  • πŸ‡«πŸ‡·France nod_ Lille

    committed followup, we can clean up this MR

  • Pipeline finished with Success
    6 months ago
    Total: 721s
    #201663
  • Status changed to RTBC 6 months ago
  • πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

    Back to RTBC once πŸ› drupalInstallModule nightwatch function does not work with Experimental modules Active was merged and the module installation process simplified.

  • πŸ‡«πŸ‡·France nod_ Lille

    Thanks

  • πŸ‡«πŸ‡·France nod_ Lille
    • nod_ β†’ committed 74cd44af on 10.4.x
      Issue #3393400 by mglaman, finnsky, m4olivei, bnjmnm, plopesc, ckrina,...
    • nod_ β†’ committed 07d570cf on 11.0.x
      Issue #3393400 by mglaman, finnsky, m4olivei, bnjmnm, plopesc, ckrina,...
    • nod_ β†’ committed 9e83195c on 11.x
      Issue #3393400 by mglaman, finnsky, m4olivei, bnjmnm, plopesc, ckrina,...
  • Status changed to Fixed 6 months ago
  • πŸ‡«πŸ‡·France nod_ Lille

    not committing to 10.3.x because of the id removal, might backport once follow-up is in.

    Committed and pushed 9e83195c5d to 11.x and 07d570cfc7 to 11.0.x and 74cd44af63 to 10.4.x. Thanks!

  • Automatically closed - issue fixed for 2 weeks with no activity.

  • Pipeline finished with Failed
    6 months ago
    Total: 1010s
    #220037
  • Pipeline finished with Canceled
    6 months ago
    Total: 275s
    #220073
  • Pipeline finished with Running
    6 months ago
    #220079
  • Pipeline finished with Failed
    5 months ago
    Total: 69s
    #234267
  • Pipeline finished with Failed
    5 months ago
    Total: 63s
    #234286
  • Pipeline finished with Failed
    5 months ago
    Total: 288s
    #234289
  • Pipeline finished with Canceled
    5 months ago
    Total: 30s
    #234291
  • Pipeline finished with Failed
    5 months ago
    Total: 219s
    #248124
  • Pipeline finished with Success
    5 months ago
    Total: 220s
    #248125
Production build 0.71.5 2024