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

Dynamic tabs: use buttons rather than links (backport to v4) #33163

Merged
merged 21 commits into from
May 24, 2022

Conversation

patrickhlauke
Copy link
Member

@patrickhlauke patrickhlauke commented Feb 20, 2021

Manual backport of #32630

@patrickhlauke patrickhlauke requested review from a team as code owners February 20, 2021 23:45
@patrickhlauke patrickhlauke changed the title Manual backport of https://github.com/twbs/bootstrap/pull/32630 Dynamic tabs: use buttons rather than links (backport to v4) Feb 20, 2021
@patrickhlauke patrickhlauke force-pushed the patrickhlauke-navs-tabs-v4 branch 2 times, most recently from 5c35626 to 68ec2f7 Compare February 20, 2021 23:58
@patrickhlauke
Copy link
Member Author

Not sure why the tests are failing ... looks like some connection problem / not spinning up Chrome 88 headless browser instance?

@XhmikosR
Copy link
Member

XhmikosR commented Feb 21, 2021

Something is wrong with the tests code changes, not sure what yet.

EDIT: try using var done = assert.async() and call done() when you expect a test to be finished.

@patrickhlauke
Copy link
Member Author

EDIT: try using var done = assert.async() and call done() when you expect a test to be finished.

apart from adding one test (literally copying it from the previous one, but with different document to test on) and making minor tweaks to the others, i've not done anything new test-wise. wonder why it's now failing when before it was working fine without any tweaks necessary?

@patrickhlauke
Copy link
Member Author

I'll need a little assist from somebody who knows how the tests stuff actually works (and why all of a sudden it doesn't anymore despite what I thought were just minimal changes ... I probably broke something along the way, but don't know enough about qUnit to work out what exactly). @rohit2sharma95 perhaps?

js/tests/unit/tab.js Outdated Show resolved Hide resolved
@patrickhlauke
Copy link
Member Author

not sure if this of any help, but even trying to run the tests locally ends up spewing up errors

02 03 2021 21:47:38.029:ERROR [karma-server]: Server start failed on port 9876: Error: Please install Chrome, Chromium or Firefox
02 03 2021 21:47:38.170:ERROR [karma-server]: Server start failed on port 9877: Error: Please install Chrome, Chromium or Firefox

Not sure if this is something more than just the few tests I've tweaked...could it be a more fundamental change/problem with karma or something?

karma-runner/karma-chrome-launcher#154

@patrickhlauke
Copy link
Member Author

FWIW I get karma failures (locally) even when trying to run npm test on the main v4-dev branch...

@ffoodd
Copy link
Member

ffoodd commented Mar 3, 2021

@patrickhlauke I guess you have Chrome or Firefox installed?
@XhmikosR could that relate to the latest change with our karma config that I had to try on Ubuntu?

@rohit2sharma95
Copy link
Collaborator

FWIW I get karma failures (locally) even when trying to run npm test on the main v4-dev branch...

I am also facing the same issue and it is not this PR specific. 🤔

@XhmikosR
Copy link
Member

XhmikosR commented Mar 3, 2021

I don't think it's related at least it works fine for me here with Chrome on Windows 10. The test failures are specific to this branch for me, like on CI. I can't comment for other OS'es. Unfortunately, we don't seem to have a debug script on v4-dev. And I cannot pinpoint which test file is causing the hang.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 3, 2021

I believe the core JS code doesn't support the new button markup. Or the markup used is wrong.

Dynamic tabs: use buttons rather than links
@XhmikosR
Copy link
Member

XhmikosR commented Mar 3, 2021

OK, NVM the above comment. The added code is not right for sure.

  1. should not fire shown when tab is disabled and show and shown events should reference correct relatedTarget are broken so I had to use QUnit.skip to temporarily run the tests
  2.  Chrome Headless 88.0.4324.190 (Windows 10) tabs selected tab should have aria-selected FAILED
             hidden tab has aria-selected = false
             Expected: "false"
             Actual: undefined
                 at Object.<anonymous> (js/tests/unit/tab.js:316:12)
                 at runTest (node_modules/qunit/qunit/qunit.js:2251:36)
                 at Test.run (node_modules/qunit/qunit/qunit.js:2239:8)
                 at node_modules/qunit/qunit/qunit.js:2461:15
                 at processTaskQueue (node_modules/qunit/qunit/qunit.js:1849:32)
                 at node_modules/qunit/qunit/qunit.js:1853:12
    
             after click, hidden tab has aria-selected = false
             Expected: "false"
             Actual: undefined
                 at Object.<anonymous> (js/tests/unit/tab.js:320:12)
                 at runTest (node_modules/qunit/qunit/qunit.js:2251:36)
                 at Test.run (node_modules/qunit/qunit/qunit.js:2239:8)
                 at node_modules/qunit/qunit/qunit.js:2461:15
                 at processTaskQueue (node_modules/qunit/qunit/qunit.js:1849:32)
                 at node_modules/qunit/qunit/qunit.js:1853:12
    
             hidden tab has aria-selected = false
             Expected: "false"
             Actual: undefined
                 at Object.<anonymous> (js/tests/unit/tab.js:324:12)
                 at runTest (node_modules/qunit/qunit/qunit.js:2251:36)
                 at Test.run (node_modules/qunit/qunit/qunit.js:2239:8)
                 at node_modules/qunit/qunit/qunit.js:2461:15
                 at processTaskQueue (node_modules/qunit/qunit/qunit.js:1849:32)
                 at node_modules/qunit/qunit/qunit.js:1853:12
    
             after second show event, hidden tab has aria-selected = false
             Expected: "false"
             Actual: undefined
                 at Object.<anonymous> (js/tests/unit/tab.js:328:12)
                 at runTest (node_modules/qunit/qunit/qunit.js:2251:36)
                 at Test.run (node_modules/qunit/qunit/qunit.js:2239:8)
                 at node_modules/qunit/qunit/qunit.js:2461:15
                 at processTaskQueue (node_modules/qunit/qunit/qunit.js:1849:32)
                 at node_modules/qunit/qunit/qunit.js:1853:12
    

@XhmikosR XhmikosR marked this pull request as draft March 3, 2021 19:05
@patrickhlauke
Copy link
Member Author

@patrickhlauke I guess you have Chrome or Firefox installed?

i was going to say "duh, of course" ... but then mulling this over further, my command line runs in WSL under windows, and no in the ubuntu subsystem that this runs, I didn't have any browsers installed. urgh. but apparently it's not non-trivial to do, so leaving this for another night...

@patrickhlauke
Copy link
Member Author

that tweak in tab.js was actually missing in v5 - added it here #33257

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

Successfully merging this pull request may close these issues.

6 participants