-
Notifications
You must be signed in to change notification settings - Fork 449
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
Parse Headless Chrome versions #263
Conversation
@elsigh May you give any timing on merging this? Thank you. |
tests/test_ua.yaml
Outdated
@@ -6657,11 +6657,17 @@ test_cases: | |||
minor: '7' | |||
patch: '10' | |||
|
|||
- user_agent_string: 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome Safari/537.36' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This testcase has been deleted. Is there a reason for this?
Please put it in again. Thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a version that would have HeadlessChrome
without a version. My regex would fail then. Can both scenarios be handled? Should there be two regexes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rogeriopvl used the UA without Version in his tests. So he got this string from that version. That is what is now fixed in the tests, and we should not change those. Indeed you'll need to change your rexeg maybe this does the job: (With this we could get rid of the family_replacement line)
- regex: '(HeadlessChrome)(?:/(\d+)\.(\d+)\.?(\d+)?)?'
Please check with current UA and your new ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@commenthol The version-less UA string now passes the regex as well.
ceb0e71
to
a1f710a
Compare
PR updated. The version-less HeadlessChrome is supported again. PTAL. |
The initial implementation of the Headless Chrome detection (ua-parser#228) didn't detect versions of the headless version. This has been fixed. A test for Ubuntu Chromium in headless mode was also added. I haven't made it recognized as HeadlessChromium as it has HeadlessChrome in the user agent. It might be worthwhile to check how a pure compiled from source Chromium behaves. Nevertheless, this commit is still an improvement. Ref ua-parser#228 Ref karma-runner/karma#2762
a1f710a
to
8d5b307
Compare
Thanks |
Bump useragent 2.3.0 to pull in ua-parser/uap-core#263 Fixes [karma-runner#2762](karma-runner#2762) Headless Chrome version detected to be 0.0.0
Bump useragent 2.3.0 to pull in ua-parser/uap-core#263 in which Headless Chrome version detected to be 0.0.0. Update tests to reflect latest useragent OS major version string identifiers. Fixes karma-runner#2762
Bump useragent 2.3.0 to pull in ua-parser/uap-core#263 in which Headless Chrome version detected to be 0.0.0. Update tests to reflect latest useragent OS major version string identifiers. Fixes karma-runner#2762
The initial implementation of the Headless Chrome detection (#228) didn't detect
versions of the headless version. This has been fixed.
A test for Ubuntu Chromium in headless mode was also added. I haven't made it
recognized as HeadlessChromium as it has HeadlessChrome in the user agent. It
might be worthwhile to check how a pure compiled from source Chromium behaves.
Nevertheless, this commit is still an improvement.
Also, a test for a bare Chrome 60 instance was added.
This PR is needed to resolve karma-runner/karma#2762