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

fix missing and extra patch_minor in ua tests #579

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

masklinn
Copy link
Contributor

patch_minor was added to regexes and some test_ua entries in #322.

Neither spec nor reference implementation were ever updated for it, so many regexes were merged (?) with a capture for patch_minor but without correctly asserting it, and a pair of cases specify a patch_minor which is not captured:

  • the facebook regex1 only has 4 capturing groups
  • same for the AWS regex2

Footnotes

  1. https://github.com/ua-parser/uap-core/blob/959e106754828ae557b0dbcfaf8eeee938d3c824/regexes.yaml#L176

  2. https://github.com/ua-parser/uap-core/blob/959e106754828ae557b0dbcfaf8eeee938d3c824/regexes.yaml#L155

`patch_minor` was added to regexes and some test_ua entries in #322.

Neither spec nor reference implementation were ever updated for it, so
many regexes were merged (?) with a capture for `patch_minor` but
without correctly asserting it, and a pair of cases specify a
`patch_minor` which is not captured:

- the facebook regex[^1] only has 4 capturing groups
- same for the AWS regex[^2]

[^1]: https://github.com/ua-parser/uap-core/blob/959e106754828ae557b0dbcfaf8eeee938d3c824/regexes.yaml#L176
[^2]: https://github.com/ua-parser/uap-core/blob/959e106754828ae557b0dbcfaf8eeee938d3c824/regexes.yaml#L155
@migueldemoura
Copy link
Member

Should we assert the key is present on all tests? Or at least for those where it's not empty.

@masklinn
Copy link
Contributor Author

masklinn commented Apr 22, 2024

Should we assert the key is present on all tests? Or at least for those where it's not empty.

I'm not sure I quite understand the question.

This PR should have filled in all the entries where the regex has a 5th capturing group (so patch_minor ends up non-empty) as they were failing the test suite I was working on (and so were the two extras for which the regex has no matching group).

So all the entries with an unspecified patch_minor are those for which it would be empty / null. Whether it's easier for those to be present or not you'd have to ask the various maintainers. For the Python implementation it shouldn't matter as it's using a set comparison for the UA cases, which handles a missing entry fine, and the Rust implementation I'm working on already handles it since that's what I used to find the problematic test cases. So personally I don't mind either way.

Also this PR should probably be linked to #562, I'd completely forgotten I'd reported that issue but hadn't had the time to work on it.

@lbarthon lbarthon merged commit cd52910 into ua-parser:master Apr 23, 2024
3 checks passed
@masklinn masklinn deleted the fix-patch-minor-ua-tests branch April 23, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants