Skip to content

Tests: Switch background image from online file to local 1x1.jpg #4866

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 24, 2021
Merged

Tests: Switch background image from online file to local 1x1.jpg #4866

merged 2 commits into from
May 24, 2021

Conversation

Krinkle
Copy link
Member

@Krinkle Krinkle commented Mar 26, 2021

Avoids noise from unexpected third-party pings during development (Little Snitch popped up unexpectedly asking for this legacy CDN hostname), and avoids console noise when testing/developing jQuery offline.

Checklist

Sorry, something went wrong.

@Krinkle Krinkle requested a review from mgol March 26, 2021 19:59
mgol
mgol reviewed Mar 26, 2021
View reviewed changes
test/data/testsuite.css
@@ -113,7 +113,7 @@ div#fx-tests div.noback {
#nothiddendivchild.prct { font-size: 150%; }

/* #9239 Attach a background to the body( avoid crashes in removing the test element in support ) */
body, div { background: url(https://static.jquery.com/files/rocker/images/logo_jquery_215x53.gif) no-repeat -1000px 0; }
body, div { background: url(1x1.jpg) no-repeat -1000px 0; }
Copy link
Member

Choose a reason for hiding this comment

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

Does it work in both Karma & PHP modes? We generally prepend baseURL to paths which suggests such URLs are not enough, see e.g.:

jquery/test/unit/css.js

Line 1134 in 025da4d

span = jQuery( "<span></span>" ).css( "background-image", "url(" + baseURL + "1x1.jpg)" );

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, the asserted value will need updating as well. Forgot about that.

Copy link
Member Author

@Krinkle Krinkle Mar 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

How come this is passing CI though?

jquery/test/unit/css.js

Lines 1568 to 1571 in 025da4d

// Firefox returns auto's value
name: "backgroundImage",
value: [ "url('test.png')", "url(" + baseURL + "test.png)", "url(\"" + baseURL + "test.png\")" ],
expected: [ "none", "url(\"https://static.jquery.com/files/rocker/images/logo_jquery_215x53.gif\")" ]

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems the expected field is never read for anything, neither applied, nor compared with, nor displayed anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've revised the PR to remove this property.

Copy link
Member

Choose a reason for hiding this comment

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

The reason for the 1x1.jpg background is outlined in the bug mentioned above: https://bugs.jquery.com/ticket/9239. Apparently, in the past we've created an additional hidden body to perform support tests in and this was causing some bugs in some browsers. The issue was fixed in ceba855 and this background was added in e5457a5.

We already have the test for that in bodyBackground.html so I guess this background was meant to just avoid some crashes in tests, perhaps related to old IE? In that case, it should be possible to just remove it. On real sites all divs wouldn't have background set anyway so removing it would get us closer to reality.

@timmywil since you added this, any objections to removing this block?

Copy link
Member

Choose a reason for hiding this comment

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

All the way back in 2011. Yea, it's probably fine to remove now.

@snyk-bot snyk-bot mentioned this pull request May 20, 2021
Closed
mgol
mgol requested changes May 24, 2021
View reviewed changes
Copy link
Member

@mgol mgol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Blocked on removing the background image.

mgol
mgol approved these changes May 24, 2021
View reviewed changes
Copy link
Member

@mgol mgol left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually, removing my objection - the path is relative to the CSS file so the fact a different absolute path is used in Karma & TestSwarm doesn't matter, the image will resolve properly.

We can handle removing the background separately.

@mgol mgol merged commit 482f846 into jquery:main May 24, 2021
@mgol mgol added this to the 3.6.1 milestone May 24, 2021
@mgol mgol added the Tests label May 24, 2021
mgol pushed a commit that referenced this pull request May 24, 2021
@Krinkle @mgol
Tests: Switch background image from online file to local 1x1.jpg
8d20cb9 
Also, remove unused `expected` property in `css` test cases.

Closes gh-4866

(cherry picked from commit 482f846)
@mgol
Copy link
Member

mgol commented May 24, 2021

Landed on main in 482f846 & on 3.x-stable in 8d20cb9.

@Krinkle Krinkle deleted the self-static branch January 28, 2022 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timmywil timmywil

@mgol mgol

Assignees
No one assigned
Labels
Tests
Milestone
3.6.1
Development

Successfully merging this pull request may close these issues.
3 participants
@Krinkle @mgol @timmywil