Repository navigation
Add every component's drift for Drift, as the specification says - #60
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For #16.
The problem
DeviceDataHashDefault.getDriftInternalreturned the smallest drift among the native results, one per component, starting fromInteger.MAX_VALUE. The specification's match metrics table describesDriftas the total difference in character positions, and 51Degrees/specifications#39 says it in full: the drift of every component's result added together. The C++ results add them (ResultsHash.cpp#L183-L192), as do Go and the Node, PHP and Python wrappers, so the same evidence gave a smallerDrifthere whenever more than one component drifted.What changes
getDriftInternaladds the results' drift, asgetDifferenceInternal,getMatchedNodesInternalandgetIterationsInternalin the same class already do. A test gives two mocked native results drifts of 3 and 5 and reads 8 fromgetDrift.Where it came from
Opened as a draft in device-detection-java as device-detection-java#614 before that repository was archived. The commit is the same, applied under
device-detection/.How it was checked
mvn -pl device-detection/device-detection.hash.engine.on-premise test -Dtest=DeviceDataTestswith JDK 17 on Windows x64, against a native library built from this repository on 2 November and the Lite data file and evidence files from the data repository: 7 tests run, 0 failures, 0 errors, 0 skipped, the newDeviceData_Drift_IsTheSumOfTheResultsamong them. The pull request job builds the native library itself and runs the same tests on every configuration.Failing first, and the full build
The first line said "Fixes #16", and a pull request into
rccloses no issue, so it now says "For #16". The rest of the text above is unchanged.DeviceData_Drift_IsTheSumOfTheResultswas run on a runner (Ubuntu 22.04, JDK 17, the native library built there, the Lite data file from the data repository) withmvn -Pdev -pl device-detection/device-detection.hash.engine.on-premise -am -Dtest=DeviceDataTests test.rcat 63d0bb8 with only the new test file added: run 42635315078.Tests run: 7, Failures: 1, withDeviceData_Drift_IsTheSumOfTheResults:112 expected:<8> but was:<3>, which is the smallest of the two drifts thatrcreturns.Tests run: 7, Failures: 0, Errors: 0, with the new test among them.The full pull request build, common-ci's
nightly-pull-request.ymlrun as a dry run from a temporary branch, built fd05a04, this branch's head, merged withrcat 63d0bb8 (run 42636362517). "Setting 'PullRequestSha'" in the logs of all 10 completed jobs is fd05a04, which is the head. Every non-macOS job passed, being Ubuntu x64 with Java 8, 11, 17 and 21, Ubuntu arm64 with Java 17, and Windows with Java 8, 11, 17 and 21, with the configure job. I cancelled the run while the six macOS jobs were still queued.macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.