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

Add JBrowser tests #17140

Merged
merged 1 commit into from
Jul 26, 2017
Merged

Add JBrowser tests #17140

merged 1 commit into from
Jul 26, 2017

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jul 15, 2017

Summary of Changes

  1. JBrowser does not correctly detect version data for Firefox browsers, this is now added
  2. JBrowser itself has no test coverage, this adds several test cases based on user agents used to test Joomla\Application\Web\WebClient to cover JBrowser::match()

Testing Instructions

For Firefox browsers, their data (especially the version) should now be detected correctly. All added test cases should pass.

@photodude
Copy link
Contributor

photodude commented Jul 16, 2017

List for UA's to consider testing for

  • CriOS
    • Mozilla/5.0 (iPad; CPU OS 10_3_2 like Mac OS X) AppleWebKit/603.1.30 (KHTML, like Gecko) CriOS/59.0.3071.102 Mobile/14F89 Safari/602.1
  • Windows RSS IE
    • Windows-RSS-Platform/2.0 (IE 11.0; Windows NT 6.1)
  • IE 11 where trident engine is 7 but IE is 11
    • Mozilla/5.0 (Windows NT 6.3; Trident/7.0; rv:11.0) like Gecko
  • IE 11 where trident engine is 7 but IE is 11 and there are things like compatibility markers before the version
    • Mozilla/5.0 (Windows NT 6.3; Trident/7.0; .NET4.0E; .NET4.0C; rv:11.0) like Gecko
  • Firefox 53
    • Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
  • Firefox other (typically developer preview releases)
    • Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.9) Gecko/20071113 BonEcho/2.0.0.9
    • Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.8) Gecko/2009033017 GranParadiso/3.0.8
    • Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2) Gecko/20100411 Lorentz/3.6.3 GTB7.0
    • Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a4pre) Gecko/20100402 Minefield/3.7a4pre
    • Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1) Gecko/20090806 Namoroka/3.6a1
    • Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b3pre) Gecko/20090109 Shiretoko/3.1b3pre
    • Mozilla/5.0 (compatible; Windows NT 5.1; en-US; rv:2.0;) Gecko/20110318 (Treco, Sub Engine) Fireweb Navigator/3.0
    • Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
  • Google bot
  • bing bot
  • curl
    • curl/7.35.0
  • Wget
    • Wget/1.15 (linux-gnu)
  • FeedBurner

Here are links to get common user agents to test with

@matrikular
Copy link
Contributor

matrikular commented Jul 16, 2017

@mbabker I've pinged you and @wilsonge for comments in #17051 (comment), which is already RTC. Apart from the "missing" test, why the new PR`?

@mbabker
Copy link
Contributor Author

mbabker commented Jul 16, 2017

I started only by writing the unit tests. When I copied over the Mozilla user agents from the web client class' test case, I found those were failing to detect the correct version and added a fix for that too. Can't say I remembered that PR being open, either way it doesn't matter what order things get merged in but the added unit test cases are required.

@matrikular
Copy link
Contributor

The problem with Firefox not being detected correctly was not only because of a missing else / if, but the regular expression for "mozilla" browsers being: preg_match('|Mozilla/([0-9.]+)|', $this->agent, $version) which is the case for almost any UA, since they start with Mozilla/X.X.

I've also looked into some alternative Mozilla ~ Firefox based browsers which all(?) have their own UA and would now be detected as "mozilla". Therefore my question #17051.

For this to work in the future, we either have to "remove" the Mozilla check, sort of what I did. or put it at the very end of that list, right?

@mbabker
Copy link
Contributor Author

mbabker commented Jul 16, 2017

I have no idea honestly, browser detection isn't something I do normally. I just did what looked to be a valid fix for the issue I ran into while importing tests. I do know without a change in JBrowser we are reporting incorrect data (something the web client class handles better right now) in that the browser version number is wrong. So clearly we need to do something, I'll leave the specifics of that to people more interested or knowledged in the area, we just need to get tests for the JBrowser class merged in at some point because right now the lines that are covered are because JHtml uses it.

@mbabker mbabker changed the title Add test for Firefox, add JBrowser tests Add JBrowser tests Jul 26, 2017
@mbabker
Copy link
Contributor Author

mbabker commented Jul 26, 2017

This is updated now to only add the unit test cases, the browser detection can be sorted in the other PR.

@wilsonge wilsonge merged commit 73577db into joomla:staging Jul 26, 2017
@wilsonge
Copy link
Contributor

Merged on review

@wilsonge wilsonge added this to the Joomla 3.8.0 milestone Jul 26, 2017
@mbabker mbabker deleted the jbrowser-testing branch July 26, 2017 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants