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

Safari 15 and 14 minor verions. Samsung Browser versions #2537

Merged
merged 7 commits into from
Nov 22, 2021

Conversation

willyaranda
Copy link
Contributor

@willyaranda willyaranda commented Nov 18, 2021

@paulrutter I have added a few UA that we are seeing in our platform that are failing on the

https://browscap.org/ua-lookup

which uses the latest library released this morning.

I'm trying to add tests, but I would love some help as I'm not familiar with php at all, and I don't know how to run them

@asgrim
Copy link
Member

asgrim commented Nov 18, 2021

@willyaranda did you check out https://github.com/browscap/browscap/wiki/Testing ? Most of the tests are just JSON files with sample UA(s) and expected output(s), should be all that is needed here I reckon. Thank you !

@willyaranda
Copy link
Contributor Author

@asgrim I have added a few local tests, but I'm unable to test it… seems the wiki is outdated? what should be the steps that needs to be done?
thanks!!

@willyaranda
Copy link
Contributor Author

ok! running like this

 /usr/local/opt/php@7.4/bin/php /usr/local/bin/phpunit -c tests/phpunit-integration.xml.dist  --no-coverage --colors tests/UserAgentsTest/V4/FullTest.php

Is there a way to select the test so it does not run the full suite if I'm debugging one?

@asgrim
Copy link
Member

asgrim commented Nov 19, 2021

Is there a way to select the test so it does not run the full suite if I'm debugging one?

I'm afraid I don't know - @mimmi20 is this possible?

@mimmi20
Copy link
Member

mimmi20 commented Nov 19, 2021

This is not possible.

@paulrutter
Copy link
Contributor

Nice work @willyaranda.
Didn't know these new minor versions where already available!

Copy link
Contributor

@paulrutter paulrutter left a comment

Choose a reason for hiding this comment

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

Can you add a few unit tests?
Those will be run automatically by Github once you push your commits :)

@willyaranda
Copy link
Contributor Author

I have pushed a few tests @paulrutter

@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #2537 (7bd757a) into 6.0.x (abceb27) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              6.0.x    #2537   +/-   ##
=========================================
  Coverage     51.02%   51.03%           
  Complexity      892      892           
=========================================
  Files          1815     1815           
  Lines         67472    67472           
  Branches      30611    30611           
=========================================
+ Hits          34429    34434    +5     
+ Misses        33043    33038    -5     
Flag Coverage Δ
full 45.85% <ø> (+0.01%) ⬆️
lite 2.11% <ø> (ø)
standard 26.51% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...er-agents/browsers/safari/safari-14-0-to-14-2.json 57.14% <ø> (ø)
...er-agents/browsers/safari/safari-15-0-to-15-4.json 85.71% <ø> (ø)
...owsers/samsung/samsung-android-browser-6-2-on.json 79.51% <ø> (+1.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abceb27...7bd757a. Read the comment docs.

Copy link
Member

@asgrim asgrim left a comment

Choose a reason for hiding this comment

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

Test failure observed in CI:

There was 1 failure:

1) UserAgentsTest\V4\StandardTest::testUserAgents with data set "issue-2537-B" ('Mozilla/5.0 (iPad; CPU OS 15_.../604.1', array('Mobile Safari 15.2', '15.2'), false, true, true)
Expected actual "Comment" to be "Mobile Safari 15.2" (was "Mobile Safari Generic"; used pattern: "mozilla/5.0*(ipad*cpu*os* like mac os x*)*applewebkit*(*khtml*like*gecko*)*version/*safari/*")
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Mobile Safari 15.2'
+'Mobile Safari Generic'

/home/runner/work/browscap/browscap/tests/UserAgentsTest/V4/StandardTest.php:197

FAILURES!
Tests: 22762, Assertions: 476688, Failures: 1.

https://github.com/browscap/browscap/runs/4286988584?check_suite_focus=true

@willyaranda
Copy link
Contributor Author

Pushed a change. iPad no longer reports the Safari version but being like a Desktop

@asgrim asgrim added this to the 6.0.50 milestone Nov 22, 2021
@asgrim asgrim self-assigned this Nov 22, 2021
@asgrim asgrim merged commit a54f895 into browscap:6.0.x Nov 22, 2021
@asgrim
Copy link
Member

asgrim commented Nov 22, 2021

Thank you @willyaranda for the update, and @mimmi20 & @paulrutter for reviews! 👍

@paulrutter
Copy link
Contributor

I released a new version of the browscap-java library: https://github.com/blueconic/browscap-java/releases/tag/1.3.10

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

4 participants