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

>= 2.9.3 breaks our tests #1437

Closed
XhmikosR opened this issue May 4, 2020 · 11 comments · Fixed by #1452
Closed

>= 2.9.3 breaks our tests #1437

XhmikosR opened this issue May 4, 2020 · 11 comments · Fixed by #1452
Assignees
Labels
Component: HTML Reporter Type: Bug Something isn't working right.

Comments

@XhmikosR
Copy link
Contributor

XhmikosR commented May 4, 2020

Tell us about your runtime:

  • QUnit version: 2.9.3 || 2.10.0
  • What environment are you running QUnit in? (e.g., browser, Node): Node.js 10.x/12.x
  • How are you running QUnit? (e.g., script, testem, Grunt): karma

When v2.9.3 was first released, I tried to update it in our Bootstrap v4-dev branch. We were getting 2 BrowserStack failures, namely iOS 11. I reverted back to 2.9.2 and forgot to report the issue 🙂 Today, I tried updating qunit to v2.10.0 and the same errors happened so I thought I'd report them.

Failed tests from our test suite:

Mobile Safari 11.0 (iOS 11.0.1) modal should restore focus to toggling element when modal is hidden after having been opened via data-api FAILED
	toggling element is once again focused
	Expected: true
	Actual: false

..........
Mobile Safari 11.0 (iOS 11.0.1) modal should not restore focus to toggling element if the associated show event gets prevented FAILED
	focus returned to toggling element
	Expected: true
	Actual: false

What are you trying to do?

git clone https://github.com/twbs/bootstrap.git -b v4-dev-xmr-deps-qunit
cd bootstrap
npm i

What did you expect to happen?

Tests to pass

What actually happened?

The aforementioned errors.


I tried pinpointing any related changes from compare/2.9.2...2.9.3 without success. Going back to v2.9.2 works fine again.
Unfortunately, running these tests locally with Chrome or Firefox works. The errors only happen on iOS Safari.

Let me know if there's something else you need!

/CC @Johann-S

@trentmwillis
Copy link
Member

Interesting; I took a look and likewise didn't see anything that looks like it would cause those failures. I might see if I can reproduce them on my iPhone when I get some time.

@trentmwillis trentmwillis added Type: Bug Something isn't working right. Type: Question Ask questions or seek assistance. labels May 5, 2020
@trentmwillis
Copy link
Member

I tried to reproduce this today. Tested on desktop Safari and mobile Safari but with more recent versions and the tests all passed. Unfortunately, I don't have access to any iOS 11 devices so I'm not sure there's anything more I could do.

@XhmikosR
Copy link
Contributor Author

Thanks for trying! It only seems to break on iOS Safari, unfortunately :/

https://github.com/twbs/bootstrap/runs/687823607#step:17:62

@trentmwillis
Copy link
Member

I tried on iOS Safari v13, so it must be that specific version of iOS Safari unfortunately. If anyone can pinpoint what's failing, I'd be happy to fix it / accept a PR.

@XhmikosR
Copy link
Contributor Author

Do you have BrowserStack tests or something similar for QUnit itself? If so, you could temporarily add iOS Safari 11. Maybe it's something on our part, but it broke after a QUnit minor release. We are still using QUnit 2.9.2 due to that.

@Krinkle Krinkle self-assigned this Jun 19, 2020
@Krinkle
Copy link
Member

Krinkle commented Jun 19, 2020

Yep, we have BrowserStack access via the JS Foundation. Looking into this now. I'll see if the tests pass on iOS/Safari 11.

@Krinkle
Copy link
Member

Krinkle commented Jun 19, 2020

It seems to pass for me.

iOS Safari 11 iOS Safari 8
Screenshot iOS Safari 11 Screenshot iOS Safari 8

@Krinkle Krinkle removed their assignment Jun 22, 2020
@XhmikosR
Copy link
Contributor Author

Thanks for testing. This seems to hint that it's not an issue with QUnit itself, but I can consistently reproduce the failure after 2.9.2:

https://github.com/twbs/bootstrap/runs/807411252#step:7:66

@Krinkle Krinkle self-assigned this Jun 25, 2020
@Krinkle Krinkle removed Status: Stalled Type: Question Ask questions or seek assistance. labels Jun 25, 2020
@Krinkle
Copy link
Member

Krinkle commented Jun 25, 2020

@XhmikosR OK, so I've checked out https://github.com/twbs/bootstrap locally and fetched the same commit as that GitHub CI run (twbs/bootstrap@6b67c96), I then ran the following in a container:

npm ci
npm run dist

... and then start a static http server on port 4000 and used http://localhost:4000/js/tests/ to run the tests in a few browsers. The BrowserStack tunnel wasn't working with iOS 11 for some reason, but I was able to reproduce the issue on iOS 10 as well:

Screenshot - failing test

The test in question has its source code at https://github.com/twbs/bootstrap/blob/6b67c96637b679f6518db5eebef1ca914040405f/js/tests/unit/modal.js#L341-L350.

  QUnit.test('should restore focus to toggling element when modal is hidden after having been opened via data-api', function (assert) {
    assert.expect(1)
    var done = assert.async()

    var $toggleBtn = $('<button data-toggle="modal" data-target="#modal-test"/>').appendTo('#qunit-fixture')

    $('<div id="modal-test"><div class="contents"><div id="close" data-dismiss="modal"/></div></div>')
      .on('hidden.bs.modal', function () {
        setTimeout(function () {
          assert.ok($(document.activeElement).is($toggleBtn), 'toggling element is once again focused')

Looking through the QUnit 2.9.2...2.9.3 diff, the following stood out to me:

+ [id^=qunit] button {
+	font-size: initial;
+	border: initial;
+	background-color: buttonface;
+ }

In particular because the Bootstrap tests involves a <button> within the qunit-fixture element.

I removed these lines from node_modules/qunit/qunit/qunit.css and refreshed, and the test is now passing for me:

Screenshot - Passing test

I'll get this fixed and backported :)

@XhmikosR
Copy link
Contributor Author

Thank you for digging into this!

@XhmikosR
Copy link
Contributor Author

We might not even need the hacks we have there (https://github.com/twbs/bootstrap/blob/v4-dev-xmr-deps-qunit/js/tests/index.html#L86) since the early days, I'll see if we need them after getting this sorted and I update to the latest QUnit release :)

It's been a long time someone touched those.

Krinkle added a commit that referenced this issue Jun 25, 2020
There are currently two places where we use buttons:
* The toolbar, with the filter "Go" button and the "Apply" and "Reset"
  buttons for the module dropdown.
* The test results, with the temporal "Abort" button.

Fixes #1437.
Krinkle added a commit that referenced this issue Jun 25, 2020
There are currently two places where we use buttons:
* The toolbar, with the filter "Go" button and the "Apply" and "Reset"
  buttons for the module dropdown.
* The test results, with the temporal "Abort" button.

Fixes #1437.
Krinkle added a commit that referenced this issue Jun 29, 2020
There are currently two places where we use buttons:
* The toolbar, with the filter "Go" button and the "Apply" and "Reset"
  buttons for the module dropdown.
* The test results, with the temporal "Abort" button.

Fixes #1437.
Closes #1452.
Krinkle added a commit that referenced this issue Jun 29, 2020
There are currently two places where we use buttons:
* The toolbar, with the filter "Go" button and the "Apply" and "Reset"
  buttons for the module dropdown.
* The test results, with the temporal "Abort" button.

Fixes #1437.
Backports #1452.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: HTML Reporter Type: Bug Something isn't working right.
Development

Successfully merging a pull request may close this issue.

3 participants