Repository navigation
Fail the example cloud tests when there is no resource key - #66
Merged
Merged
Conversation
The cloud tests under examples/ skipped when no resource key was set, so a run with no key reported success having called nothing. Each now asks its KeyHelper for the key, which reads _51DEGREES_RESOURCE_KEY, then _51DEGREES_RESOURCE_KEY_BESPOKE, then the names the example reads today, each as an environment variable and then as a system property, and fails the test naming every name tried, the product the key has to carry, how to see what a key carries and where to create one. KeyHelper.getOrSetResourceKey passed a null key to System.setProperty, which throws a NullPointerException, whenever no key was found and it was asked not to throw. It now keeps only a valid key. The Device Detection web cloud test sends a known User-Agent and checks that the page shows the browser and platform the cloud engine detected, rather than accepting any 200 response.
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 #64.
The problem
The cloud tests under
examples/skipped when no resource key was set, so a run with no key reported success having called nothing. Separately,KeyHelper.getOrSetResourceKeyin bothexamples/device-detection/shared/.../KeyHelper.javaandexamples/ip-intelligence/shared/.../KeyHelper.javapassednulltoSystem.setPropertywhen it found no key and was told not to throw, which throws aNullPointerException.The open drafts #59 and #57 do the same for the pipeline modules and the IP Intelligence cloud module. Neither touches
examples/, and this pull request touches nothing else.What changed
The
setPropertycall is guarded.getOrSetResourceKeynow keeps a key only whenKeyUtils.isInvalidKeysays it is valid, so a missing or invalid key is never stored.The tests fail rather than skip. Each
KeyHelpergainsrequireTestResourceKey, and the Device Detection one alsorequireResourceKeyfor the TAC and native model tests, which readSuperResourceKey. They read_51DEGREES_RESOURCE_KEY, then_51DEGREES_RESOURCE_KEY_BESPOKE, then the names the file reads today in their current order, each as an environment variable, as its upper case name and then as a system property. Anything that is not a key, such as the empty string the example poms pass for an unset key, is passed over. With no key they fail with the wording agreed for all four languages.GettingStartedCloudTest,MetadataCloudTest,MinimalExampleTest,GettingStartedWebCloudTestTestResourceKeydeviceNativeModelCloudTest,TacCloudTestSuperResourceKeyhardwareGetAllPropertiesCloudTest,GettingStartedCloudTest,MetadataCloudTest,GettingStartedWebCloudTest51DEGREES_RESOURCE_KEY,TestResourceKeyipGettingStartedCloudMixedTest,GettingStartedWebCloudMixedTest51DEGREES_RESOURCE_KEY,TestResourceKeyipanddeviceThe two IP Intelligence web cloud tests were missing from the issue's table but skipped in the same way, so they are included.
The Device Detection web cloud test checks the page.
GettingStartedWebCloudTestsends a Chrome 98 on Windows User-Agent and checks that the page says the cloud device detection engine detected the values, and that Browser Name is Chrome and Platform Name is Windows. A row that shows no value fails with "The cloud answered without BrowserName (reason), which is what a key that does not carry the device product gets."Unit tests. A new
KeyHelperTestin each examplessharedmodule covers the no key path ofgetOrSetTestResourceKey(false), the failure message word for word, the order the names are read in, and that a value that is not a key is passed over.The configure.51degrees.com link in the message carries the repository's campaign tags, and common-ci
scripts/utm-lint.ps1reports the tree clean for51degrees-java.Variable order and #59
The order follows the decision for .NET, Node, Python and Java, which is the two shared names first and then the file's own names. Two differences are worth knowing for review.
LiveCloudKeysreads_51DEGREES_RESOURCE_KEY,RESOURCE_KEY,_51DEGREES_RESOURCE_KEY_BESPOKE,TestResourceKey,SUPER_RESOURCE_KEY, so it putsRESOURCE_KEYsecond and differs from the other languages on order. This pull request does not change Make every cloud test call the live service and fail when it cannot #59._51DEGREES_RESOURCE_KEYholding a key without thehardwareproduct is used ahead of aSuperResourceKeythat has it. CI sets neither shared name (the Build and Test log prints "No keys matching '_51DEGREES_RESOURCE_KEY*' were supplied"), so CI is unaffected.How to verify
All runs are on
ubuntu-latestwith JDK 17, testingexamples/device-detectionandexamples/ip-intelligenceagainst the published 4.5.130 packages, once with no key and once with the bespoke key passed as-DTestResourceKeyand-DSuperResourceKey, asci/run-unit-tests.ps1does.rcat d4abce9 plus the no key unit test: run 423930691971. With no key, every cloud test fails naming the variables and the product, none skips, and none ends with a
NullPointerException.On
rcall 12 skipped with "Skipping test, no resource key found". Under Maven the three Device Detection tests named in the issue skip rather than throw, because the example poms passTestResourceKeyon as an empty string (the surefire report listsTestResourceKey=''), which is notnull. The exception comes when the property is absent, which step 2 shows.On this branch all 12 fail, for example:
The 10 that pass are the
KeyHelperTestcases.2.
KeyHelper.getOrSetTestResourceKey(false)with no key set gives noNullPointerException, andSystem.getProperty("TestResourceKey")is stillnullafterwards.getOrSetTestResourceKey_NoKey_StoresNothingclears the system property for the test and checks the environment does not set it. Onrcit ends withjava.lang.NullPointerExceptionin both modules. On this branch it passes in both, in both jobs, and did not skip.3. With the bespoke key, the
device,ipandhardwaretests pass.A step that never prints the key asked
accessiblepropertieswhat the key carries:On this branch all 22 passed, the 12 cloud tests including TAC, native model and the page check of the web test.
What remains
answeredWithout_NoReason_SaysSo.${TestResourceKey}from their configuration files, which looks at the environment before system properties. A developer who exports_51DEGREES_RESOURCE_KEYand a differentTestResourceKeygets the test's key lookup and the page's key from different names.Full build
The branch has
rcmerged in at 2299507, which brings #52, #54 and #57 and touches none of these files. common-ci's pull request build, run as a dry run with the bespoke key, built and tested da6b7e4 in all 15 configurations and every job passed (run 42397586757). In the Ubuntu Java 17 job the 12 cloud tests and bothKeyHelperTestclasses ran with none skipped, for exampleTests run: 1, Failures: 0, Errors: 0, Skipped: 0 ... in fiftyone.devicedetection.examples.console.TacCloudTest.