Repository navigation
Check what the umbrella getting started examples answer, and test a data file reload - #108
Merged
Merged
Conversation
…ngine keeps its Property list across a data file reload
…r on its data file
jwrosewell
marked this pull request as ready for review
November 14, 2026 09:25
This was referenced Nov 14, 2026
Draft
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.
Problem
Two test gaps, with no package code to change.
device-detection/fiftyone.devicedetection/examples/gettingstarted.test.jslines 51 and 64 to 71 at 758fea81 evaluates the example's code and ends the test after four seconds whatever it printed, andip-intelligence/fiftyone.ipintelligence/examples/gettingstarted.test.jslines 27 to 30 only requires the module, so its answer is printed, if at all, after the test has ended. This was left under "What remains" in #95.refreshDataorreloadfinds nothing. The C library pins what a reload must do for an engine built with no Property names, inHashPositionsReloadTests.NothingNamed_SmallThenLarge(test/dd/hash/HashPositionsTests.cppline 530 at 51degrees-cxx 7c4ac436). There the engine keeps the Property list it started with and reads each of those Properties from the reloaded file, where they sit at other positions.What the specification says
device-detection-specification/required-examples.mdlines 14 to 23 at c8128ca4:What changed
truefor the iPhone User-Agent andfalsefor the Windows one, and the IP Intelligence one must name a country for 8.8.8.8. The country depends on the cloud service's data, which today answers "Pakistan" where the example's comment expects "United States", so the test only asks for a name rather than a message saying why there is none. With no key each must say how to create one. CI always sets a key, so the runs below take the first branch.device-detection/fiftyone.devicedetection.onpremise/tests/reloadProperties.test.jsbuilds an engine with no Property names on a copy of the free Lite data file, records its Property list and itsIsMobile,PlatformNameandBrowserNamefor an iPhone User-Agent, replaces the copy with the Enterprise data file CI fetches, refreshes the data file as the update service does, and checks that the engine now holds the Enterprise data set, keeps the same Property list and gives the same three values.IP Intelligence has no pair of free data files with the same Properties (the ASN file has
AsnandAsnNameand the Lite file has the registration Properties), so its reload stays covered by the C library'sIpiPositionsReloadTests.Before this test was written, a throwaway run printed what a reload does to each engine (run 43559640564). The Device Detection engine on the Lite file, reloaded onto the Enterprise file, had 284 Properties in its metadata and still 15 available, as on the Lite file. The IP Intelligence engine on the ASN file, reloaded onto the Lite file, had 3 with none available, as neither of its two is in the Lite file. Both match the C library's rule.
Tests
Each ran on one Ubuntu runner with Node.js 24, with the native modules built, the data files fetched as CI fetches them and the CI resource key, in a throwaway workflow that is not part of this draft.
Red, run 43563068117, on
rcwith the Device Detection umbrella example answering the opposite of what it detects and the IP Intelligence one printing an empty line for the country. The old tests passed 2 of 2, and the new ones then failed 2 of 2, with "Expected: "true" Received: "false"" and an empty line where a country name was expected.Green, run 43563058024 at 24c65ee6, the same as this head but for the wording of a comment and an unused
requirethe lint refused, where both umbrella tests passed. The reload test ran there and failed, 1 failed and 2 passed, because the distributor did not serve the free Lite data file at the time ("The free data file (lite-hash-v41) is not available at present"). The reload test passed in run 43560740891 at 43d2d0c3. That same run failed the IP Intelligence umbrella test, 1 failed and 2 passed, with the country then fixed as "United States" and the cloud service answering "Pakistan", which is why the test now asks only for a name. 481b5fc later changed the reload test to build one Pipeline with no file system watcher, as below, so the head's reload test is proven by the full build.The first two full builds of this draft failed. Run 43563687773 failed the lint on an unused
requireofpathin both umbrella tests, which 423be61 removes. Run 43569318892 passed on Linux and failed on Windows after the tests had run, because the reload test's Pipeline registered the engine with an update service whose file system watcher fired on the copied file and raisedEPERMonce the test removed its folder. 481b5fc7 builds one Pipeline withfileSystemWatcher: false, as the test refreshes the file itself.The reload test passes on
rctoo, as nothing in the engine changes here. It is a check that a later change to the engine or the C library cannot break the reload without a test failing, and the red above covers only the umbrella tests.Full build
Run 43573142555 on
tmp/pr-matrix-108-c, started bytools/pr-matrix.pywithout macOS. All 7Setting 'PullRequestSha'lines give this head, 481b5fc, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,389 unit tests with 2 skipped and 145 integration tests with 4 skipped, the reload test and both umbrella tests among them.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.
What remains