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 failing tests #7564

Closed
wants to merge 3 commits into from
Closed

Fix failing tests #7564

wants to merge 3 commits into from

Conversation

philippschulte
Copy link

Description:

I am using a private fork of the rust version which is based on https://github.com/matomo-org/device-detector/releases/tag/6.2.1. The rust project runs every single test and the once listed in this PR are failing. I would highly appreciate if you would concider the changes of this PR for your next release candidate so that I don't have to continue fixing the tests when upgrading to the next version. Thanks in advance!

Review

@liviuconcioiu
Copy link
Collaborator

Hi @philippschulte. Tests are valid. Fire OS version is 7, because is based on Android 9. And the other test is an Android browser, that fakes the user-agent as iPad.

@mindreader can you look into this?

@sanchezzzhak
Copy link
Collaborator

client-hints not working for IOS/MACOS/IPADOS, Therefore, if they exist, then you should not trust that this is an Apple user agent.
http-x-xml-request header not send for IOS/MACOS/IPAD OS this header is only sent by android from apps or browsers

so yes the tests are correct

@mindreader
Copy link
Contributor

There are always code changes to be pulled into the rust version to bring it up to parity.

I did my best to structure the code similarly to make it easier to port the logic over, because there is no avoiding it.

I was intending to bring it up to parity soon, when I have some time.

@sanchezzzhak
Copy link
Collaborator

https://github.com/sanchezzzhak/node-device-detector/blob/master/tests/tools/update-fixtures.js

this is my data update file from matomo/device-detector to node js. this allows me to save time and not perform manual copying.
all I do is run the tests, if they don't pass, I look at what has changed and make corrections. I recommend doing something similar in your project.

@philippschulte
Copy link
Author

philippschulte commented Jan 24, 2024

Thanks everybody for confirming that the tests are correct! I'll compare the php and the rust code base and incorporate the necessary changes.

Thanks for the great work you are doing here! I really appreciate it. Feel free to close this one.

@mindreader
Copy link
Contributor

@philippschulte I tagged a new updated version a few minutes ago.

As for your private fork, I understand the need to do hacky business things sometimes, but I would appreciate if you have made any performance enhancements that you consider contributing back. Maintaining and constantly updating from upstream is not really something you want to do all by yourself every few months as our code bases begin to diverge.

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.

4 participants