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

Feature/2534 update apple platform versions #2535

Merged

Conversation

paulrutter
Copy link
Contributor

#2534 Update Apple platform versions

  • Added 'OSX_dynamic' and 'OSX_10_16' for Mac OS
  • Added 'ipadOS_15_0' / 'ipadOS_15_2'
  • Added 'iOS_A_15_0' / 'iOS_A_15_2'
  • Added platform to the browsers that it runs on, also including the "generic" platforms to have better coverage for future versions (when the useragent remains the same)

@asgrim @mimmi20
I did an attempt to add the recent Apple platform versions to browscap.
Also added a "dynamic" OSX item in platforms.json

I still don't exactly understand why those "dynamic" platforms are not added to all browsers, since that would help recognizing future versions which are not specified exactly in "platforms.json". I did that now for the Apple related platforms, but not sure if that's right. Can you help me on this logic? Maybe i'm misunderstanding and those generic ones shouldn't be added?

ocluf and others added 12 commits April 29, 2021 15:12
updating from browsecap
Revert "updating from browsecap"
Revert "Revert "updating from browsecap""
- Added 'OSX_dynamic' and 'OSX_10_16' for Mac OS
- Added 'ipadOS_15_0' / 'ipadOS_15_2'
- Added 'iOS_A_15_0' / 'iOS_A_15_2'
- Added platform to the browsers that it runs on, also including the "generic" platforms to have better coverage for future versions (when the useragent remains the same)
@paulrutter
Copy link
Contributor Author

paulrutter commented Nov 12, 2021

Also, can you help determine which unit test if failing?
I cannot see that in the logs: https://github.com/browscap/browscap/runs/4187437561?check_suite_focus=true

Could it be the "dynamic" platforms that i added to the browser files?

@paulrutter
Copy link
Contributor Author

paulrutter commented Nov 12, 2021

I fixed the comments, but i think that won't help solve the unit test issue.
Can you take a look at my question here? #2535 (comment) and #2535 (comment)

@paulrutter
Copy link
Contributor Author

Thanks for approving @mimmi20, but since the unit tests still fail i don't think we should merge.
I need more info on the "dynamic" platforms, as mentioned earlier.

@mimmi20
Copy link
Member

mimmi20 commented Nov 12, 2021

"dynamic" platforms only work, if the platform version matches the browser version.

@mimmi20
Copy link
Member

mimmi20 commented Nov 12, 2021

The Build is failing since May. Idont think your changes caused this.

@mimmi20 mimmi20 linked an issue Nov 12, 2021 that may be closed by this pull request
- Attempt to fix unit test by reducing expectation to 1
@paulrutter
Copy link
Contributor Author

The Build is failing since May. Idont think your changes caused this.

I will try to fix the unit tests first, so we can actually see what my changes did to existing functionality.

- Attempt to fix unit test by reducing expectation to 1
- Attempt to fix unit test by reducing expectation to 1
- Attempt to fix unit test by reverting to any
- Update status badge for CI integration
- Revert dynamic platforms, as these don't work
- Add safari 15 as that's included with Mac OS X 15
@paulrutter
Copy link
Contributor Author

paulrutter commented Nov 15, 2021

@mimmi20 I managed to get the unit tests running again, by reverting a change in https://github.com/browscap/browscap/pull/2535/files#diff-cebc14b7503943d2278cbaf4fe86c60f57707b619511f54041399054709dd28d.

Now i had to fix few other unit tests that were added already, but didn't succeed.
I've tried, to my best knowledge, to add the new Apple platforms:

  • Mac OS Big sur (11) and Monterey (12), 10.x range is no longer used although 10.16 was used at some point
  • iOS 14.0 / 14.8 and 15.0 / 15.2 (which is in beta)
  • iPad OS 15.0 / 15.2, didn't add 14.3 / 14.8 as i wasn't sure it was used

I also added Safari 15 specifically, as that is shipped with MacOS Monterey.
In the readme, i updated the badge for the CI, which was no longer ok.

@mimmi20 i processed your latest comments. Can you re-review and merge if you think it's ok now?
The code coverage report still seems a bit off, but not sure how to resolve that.

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #2535 (ce40a85) into 6.0.x (82b8017) will decrease coverage by 26.37%.
The diff coverage is 35.29%.

Impacted file tree graph

@@              Coverage Diff              @@
##              6.0.x    #2535       +/-   ##
=============================================
- Coverage     77.40%   51.02%   -26.38%     
  Complexity      892      892               
=============================================
  Files            65     1815     +1750     
  Lines          3124    67472    +64348     
  Branches          0    30611    +30611     
=============================================
+ Hits           2418    34428    +32010     
- Misses          706    33044    +32338     
Flag Coverage Δ
full 45.83% <31.37%> (?)
lite 2.11% <12.74%> (?)
standard 26.50% <30.39%> (?)

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

Impacted Files Coverage Δ
...-agents/browsers/chrome/headless-chrome-60-on.json 70.00% <0.00%> (ø)
...gents/browsers/chrome/headless-chrome-generic.json 20.00% <0.00%> (ø)
...wsers/coc-coc-browser/coc-coc-browser-generic.json 0.00% <0.00%> (ø)
...ser-agents/browsers/edge-browser/edge-generic.json 6.45% <0.00%> (ø)
...s/browsers/firefox-on-desktop/firefox-generic.json 34.35% <0.00%> (ø)
...nts/browsers/firefox-on-ios/firefox-ios-15-on.json 76.92% <0.00%> (ø)
...s/browsers/firefox-on-ios/firefox-ios-generic.json 0.00% <0.00%> (ø)
...nts/browsers/mobile-safari/mobile-safari-12-x.json 70.37% <ø> (ø)
...nts/browsers/mobile-safari/mobile-safari-13-x.json 92.59% <ø> (ø)
.../browsers/mobile-safari/mobile-safari-generic.json 40.16% <0.00%> (ø)
... and 1765 more

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 82b8017...ce40a85. Read the comment docs.

@paulrutter
Copy link
Contributor Author

@mimmi20 can you merge and release a new browscap version if you're ok with the changes?

@mimmi20
Copy link
Member

mimmi20 commented Nov 16, 2021

@paulrutter I may be able to merge, but not to release.
@asgrim Could you merge and release?

@asgrim asgrim merged commit 2c74b27 into browscap:6.0.x Nov 17, 2021
@asgrim asgrim self-assigned this Nov 17, 2021
@asgrim asgrim added this to the 6.0.48 milestone Nov 17, 2021
@asgrim
Copy link
Member

asgrim commented Nov 17, 2021

Yep, thank you @paulrutter - appreciate the effort and fixing up those tests 🤘

@paulrutter paulrutter deleted the feature/2534-Update-Apple-platform-versions branch November 17, 2021 08:24
@paulrutter
Copy link
Contributor Author

@asgrim do we need to trigger something for http://browscap.org/ as well?

@asgrim
Copy link
Member

asgrim commented Nov 17, 2021

Yes, waiting for Dependabot

@mimmi20 FYI the release procedure is at https://github.com/browscap/browscap/wiki/Public-release-procedure :) I believe you should be able to do most of it now

@asgrim
Copy link
Member

asgrim commented Nov 17, 2021

@paulrutter looks like it is up now 👍

@paulrutter
Copy link
Contributor Author

Yes, great!

@willyaranda
Copy link
Contributor

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/15.1 Safari/605.1.15

is incorrectly detected, as does not match. Changing Version/15.1 for Version/15.0 does match and get the major version properly

@paulrutter
Copy link
Contributor Author

Hmm i wasn't aware of safari 15.1 was already released. We might add that as well.

@paulrutter
Copy link
Contributor Author

Created #2536 @willyaranda

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.

Update Apple platform versions
5 participants