[jwrosewell]

The problem

The Cloud 51Did Tests
check fails on rc. Run 43130812995,
job 130894182769, on headSha 9a668883, which is rc's head and this
draft's base, ends with

[ERROR] Tests run: 6, Failures: 0, Errors: 1, Skipped: 0 ... in fiftyone.pipeline.did.DidClientLiveTests
[ERROR]   DidClientLiveTests.anIdentifierForAnotherAddress_ReportsEveryFactor:210->identifiersCreatedFor:371 » NullPointer
[ERROR] 51Degrees :: Pipeline :: 51Did ..................... FAILURE

In DidClientLiveTests.java
the field licenceKey is declared at line 64, init() at lines 67 to 75
assigns resourceKey and never assigns licenceKey, and line 371 then
reads the field to build the form body

String form = "resource=" + DidClient.encode(resourceKey)
    + "&license=" + DidClient.encode(licenceKey)

so encode is handed null. The field is written nowhere and read in that
one place, inside identifiersCreatedFor, which only
anIdentifierForAnotherAddress_ReportsEveryFactor calls.

Where it came from

The field was assigned until commit
d4d4dc6,
"Make every cloud test call the live service and fail when it cannot
(#59)". Its parent had

licenceKey = env("_51DEGREES_LICENSE_KEY", "LICENSE_KEY");
client = new DidClient(resourceKey, licenceKey);

and d4d4dc6 moved the lookup from env to
LiveCloudKeys.find
and inlined it into the constructor call, which left the field behind.

There is no specification in this. The behaviour under test is what the
cloud service answers, and the fault is the test's own field, so the source
of truth here is the file's own two readers of the licence key, which are
the client and the form body, and those are what the change makes agree.

What changed

Four lines added and two removed, in one test file.

// One lookup serves both, so the key the form body sends and the
// one hasLicenceKey() answers for can never disagree.
licenceKey = LiveCloudKeys.find(LiveCloudKeys.LICENCE_KEY_NAMES);
client = new DidClient(resourceKey, licenceKey);

That is the shape the file had before d4d4dc6, with one lookup feeding both
the field and the client.

Why no failure message for a missing licence key

A NullPointerException 300 lines from init() says nothing about a
missing secret, so it is worth asking whether init() should fail with a
clear message when the licence key is absent, the way it already does for a
missing resource key with fail(LiveCloudKeys.noResourceKey()). It should
not, for two reasons.

The licence key is deliberately optional where the resource key is not.
LiveCloudKeys says so at lines 53 to 57, "A licence key is optional,
because an account can hold its products on the resource key alone", and
the workflow says the same at line 59, "Without a license key the tests
that redeem a sealed verdict skip". Both tests that need one already open
with Assume.assumeTrue("Set _51DEGREES_LICENSE_KEY to redeem a sealed result.", client.hasLicenceKey()), so the clear message for a missing
licence key exists already and it is a skip rather than a failure. A
fail() would turn that designed skip into a red job for every account
that holds its products on the resource key alone.

The green run below shows that half is live rather than theoretical. The
job runs the test class once for each resource key, and the second key,
_51DEGREES_RESOURCE_KEY_BESPOKE, has no licence key paired with it, so
its half logs "License key: none" and skips those two tests. A fail()
would have made that half red.

The second reason is that the fix removes the way the field could be null
where it is read. The field and client.hasLicenceKey() now come from the
same lookup, and LiveCloudKeys.find returns either null or a trimmed
non-empty value, so when there is no licence key the guard skips the test
before line 371 is reached, and when there is one the field holds it.

How it was tested

Red and green are the same workflow over the same test class, and they
differ in one line.

• Red. Run 43130812995
  on headSha 9a668883, this draft's base. With
  _51DEGREES_RESOURCE_KEY_51DID, which the log shows paired with
  _51DEGREES_LICENSE_KEY_51DID, "Tests run: 6, Failures: 0, Errors: 1,
  Skipped: 0" and BUILD FAILURE. With _51DEGREES_RESOURCE_KEY_BESPOKE,
  "License key: none", "Tests run: 6, Failures: 0, Errors: 0, Skipped: 2"
  and BUILD SUCCESS.
• Green. Run 43138082682
  on headSha 8176b622, this draft's head. With
  _51DEGREES_RESOURCE_KEY_51DID, "Tests run: 6, Failures: 0, Errors: 0,
  Skipped: 0" and BUILD SUCCESS. The Skipped: 0 is the part that matters,
  because it says the test that died ran against the live service and
  passed rather than being skipped over. The _51DEGREES_RESOURCE_KEY_BESPOKE
  half is unchanged at "Skipped: 2" and BUILD SUCCESS.

The green run was started by hand with workflow_dispatch on the branch.
The workflow's pull_request trigger is branches: [main], so a draft
based on rc never starts it, which is also why this fault reached rc
and only showed when #112
opened rc into main.

Full build

Run 43138137079,
started at 01:58 UTC on the build branch tmp/pr-matrix-113 at 0205cf17,
finished green. Configure, the Complete job and all nine Build and Test jobs
are a success, those being Ubuntu on Java 8, 11, 17 and 21, Ubuntu ARM on
Java 17, and Windows on Java 8, 11, 17 and 21. The two Compare Performance
jobs are skipped, as they are on every draft of this kind. All ten
Setting 'PullRequestSha' lines in the logs read
8176b622ae5abde121a1abc431da6cdec6ed83a0, which is this draft's head, so
every job built this code and not something else.

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. Configure printed
"Left out 6 macOS entries".

What this does not touch

Test code only, in one file, so nothing a user sees changes and there is
nothing for the release note.

Ben Shillito's #56

#56, "Create the
51Did for another address as a licensed caller in the live test", changes
this same file and no other, so it is worth saying plainly that it is not
touched here and that there is no clash. Merging pull/56/head into this
branch in a clone auto-merges and leaves the file identical to this branch,
so the merge is a no-op either way round. Everything #56 proposes is
already on rc, brought in by d4d4dc6, which carries
identifiersCreatedFor, the POST form body, readIdentifiers and the same
wording in the comment above the test. What #113 adds is the one field
assignment d4d4dc6 dropped on the way. #56 is left open, unchanged, for its
author to decide on.

Still to do

• Nothing on the code. The head is 8176b622 and this body was written after
  every run above had finished, on that head.
• The build branch tmp/pr-matrix-113 is still there at 0205cf17. Deleting
  a branch is refused to the sign-in that started the build, so whoever
  merges this draft should delete it.
• Java's release draft #112
  has to build again once this merges, because its current build is on
  rc at 9a668883.