Skip to content

uts/rest/unit housekeeping: a leaked local path, six untitled sections, a duplicated test, three misfiled Realtime tests, and three header/spec-point claims that do not hold #532

Description

@owenpearson

Summary

Seven small, independent faults found while deriving uts/rest/unit into ably-python. None of them is interesting on its own and none needs a normative ruling except where noted; they are collected here so they can be swept in one pass rather than filed seven times. Ordered easiest first. All line references are against d9a04ca, paths relative to uts/.

1. A local filesystem path leaks into six specs

Six specs point their mock-infrastructure reference at a path on someone's laptop:

These tests use the mock HTTP infrastructure defined in /Users/paddy/data/worknew/dev/dart-experiments/uts/rest/unit/rest_client.md.

All six at line 10:

  • rest/unit/types/paginated_result.md:10
  • rest/unit/channel/publish.md:10
  • rest/unit/channel/history.md:10
  • rest/unit/channel/idempotency.md:10
  • rest/unit/encoding/message_encoding.md:10
  • rest/unit/presence/rest_presence.md:10

(Three say "defined in", three "described in"; the path is byte-identical in all six.)

The intended target is the sibling spec uts/rest/unit/rest_client.md. Two reference conventions already exist in the corpus: files in uts/rest/unit/ itself write the bare name (time.md:10, logging.md:10, request.md:10, batch_presence.md:10, batch_publish.md:10 — "defined in rest_client.md"), and files one level down write the repo-relative path (channel/annotations.md:10, channel/get_message.md:10, channel/publish_result.md:10, channel/message_versions.md:10, channel/update_delete_message.md:10 — "defined in uts/rest/unit/helpers/mock_http.md"). All six offenders are one level down, so the fix matching their neighbours is to replace the absolute path with uts/rest/unit/rest_client.md.

2. Six sections carry no Test ID

uts/docs/writing-test-specs.md:28 and :55:

Every test in the UTS suite has a unique identifier. The ID appears explicitly in the spec markdown and must be included as a comment in every derived (language-specific) implementation.

Add a **Test ID** line immediately after the test heading

Scanning every ## test section under uts/rest/unit turns up six without one — these are the complete set:

Location Heading Suggested ID
rest/unit/encoding/msgpack_interop.md:24 RSL6a3 - Decode binary-encoded protocol messages using interop fixtures rest/unit/RSL6a3/msgpack-fixtures-decode-0
rest/unit/encoding/msgpack_interop.md:83 RSL6a3 - Re-encode decoded messages back to msgpack (round-trip) rest/unit/RSL6a3/msgpack-fixtures-round-trip-1
rest/unit/channel/annotations.md:280 RSAN2a — delete sends POST with ANNOTATION_DELETE rest/unit/RSAN2a/delete-post-annotation-delete-0
rest/unit/channel/annotations.md:331 RSAN3b — get sends GET to correct endpoint rest/unit/RSAN3b/get-sends-get-0
rest/unit/channel/annotations.md:382 RSAN3c — get returns PaginatedResult of Annotations rest/unit/RSAN3c/get-returns-paginated-result-0
rest/unit/channel/annotations.md:459 RSAN3b — get passes params as querystring rest/unit/RSAN3b/get-params-querystring-1

The suggestions follow the file's own six existing ids (annotations.md:16, 43, 102, 135, 182, 236), including the -0/-1 indexing already used there for the two RSAN1c4 tests.

3. Two byte-identical RSC17 tests in rest_client.md

rest/unit/rest_client.md:462 and :484 — two ## RSC17 - ClientId Attribute headings. diff of the two sections reports one difference:

< **Test ID**: `rest/unit/RSC17/client-id-from-options-0`
---
> **Test ID**: `rest/unit/RSC17/client-id-matches-auth-1`

Heading, spec-requirement line, setup and both assertions (:478-479 and :500-501) are identical:

ASSERT client.clientId == "explicit-client-id"
ASSERT client.clientId == client.auth.clientId

The second slug says what it was meant to be — the first test should presumably construct with clientId and check client.clientId, the second should check that Auth#clientId agrees (or, more usefully, that it is populated from a source other than ClientOptions, e.g. a token's clientId). Either write that test or drop the duplicate section; as it stands the suite counts the same assertion twice.

4. Three Realtime tests filed under rest/unit

rest/unit/fallback.md:1415, :1458 and :1503 — the REC3a, REC3b and REC3 sections — construct a Realtime client and exercise the connection:

client = Realtime(options: ClientOptions(key: "appId.keyId:keySecret"))
result = AWAIT client.connection.checkConnectivity()

(fallback.md:1434, :1441, :1473, :1481, :1528-1529, :1545-1546, :1562-1563.) Their ids are rest/unit/REC3a/default-connectivity-check-url-0, rest/unit/REC3b/custom-connectivity-check-url-0 and rest/unit/REC3/connectivity-check-validation-0, in a directory uts/README.md:10 describes as "REST unit tests (mocked HTTP)", alongside :21 "Realtime unit tests (mocked WebSocket)". fallback.md:1422 already concedes the point: "This test is primarily relevant for Realtime clients that perform connectivity checks."

Proposal: move the three sections to uts/realtime/unit/ and reindex their ids to the realtime/unit/… category, dropping REC3, REC3a and REC3b from the fallback.md:3 header at the same time. A REST-only SDK currently has to skip them or fake a Realtime client to satisfy the category it is in.

5. channels_collection.md:3 claims RSN3b and RSN3c but nothing tests them

rest/unit/channels_collection.md:3:

Spec points: RSN1, RSN2, RSN3a, RSN3b, RSN3c, RSN4a, RSN4b

grep -rn 'RSN3b\|RSN3c' uts/ returns exactly one line — that header. Both points are about the two-argument form, specifications/features.md:284-285:

  • (RSN3b) If options are provided, the options are set on the RestChannel
  • (RSN3c) Accessing an existing RestChannel with options in the form Channels#get(channel, options) will update the options on the channel and then return the existing RestChannel object. (Note that this is soft-deprecated and may be removed in a future release, so should not be implemented in new client libraries. …)

Every get call in the file is single-argument (channels_collection.md:54, 93, 94, 95, 126, 153, 154, 181, 209, 265, 269); no get(name, options) call exists anywhere in uts/.

Either add the tests or drop the two points from the header. Worth noting for RSN3c specifically that features.md calls it soft-deprecated and says it "should not be implemented in new client libraries", so a test for it would need an applicability note if it is written at all.

6. rest_client.md:3 claims RSC7b, which is superseded

rest/unit/rest_client.md:3:

Spec points: RSC5, RSC7, RSC7b, RSC7c, RSC7d, RSC7e, RSC8, …

specifications/features.md:96:

  • (RSC7b) (Please note this clause and the associated header have now been superseded by RCS7d) The header X-Ably-Lib: [lib][.optional variant]?-[version] should be included in all REST requests to the Ably endpoint …

So this is not a missing test — a test for RSC7b would be wrong to write. The claim should just come off the header. (ably-pubsub-js already dropped it: test/uts/rest/unit/rest_client.test.ts:4 lists RSC5, RSC7, RSC7c, RSC7d, RSC7e, RSC8a-c, RSC17, RSC18.)

The same header also claims the bare RSC7, which is likewise untested as such — but RSC7 is an umbrella whose children RSC7c/d/e are all tested, so listing it is defensible and is not part of this ask.

7. rest_client.md:546 (RSC18) over-specifies when the failure occurs

rest/unit/rest_client.md:546-566, rest/unit/RSC18/basic-auth-over-http-rejected-1:

# No mock needed - should fail before making request
…
Rest(options: ClientOptions(
  key: "appId.keyId:keySecret",
  tls: false
)) FAILS WITH error
ASSERT error.code == 40103 OR error.message CONTAINS "insecure" OR error.message CONTAINS "TLS"

Neither governing clause imposes a timing. specifications/features.md:109:

  • (RSC18) If ClientOptions#tls is true, then all communication is over HTTPS. If false, all communication is over HTTP however Basic Auth over HTTP will result in an error as private keys cannot be submitted over an insecure connection. See Auth below

and specifications/features.md:179:

  • (RSA1) Basic Auth connects over HTTPS by default. Any attempt to use Basic Auth over HTTP without TLS will result in an error

"Any attempt to use" is satisfied by raising at request time. ably-python does exactly that — ably/http/http.py:183-187:

if self.auth.auth_mechanism == Auth.Method.BASIC and self.preferred_scheme.lower() == 'http':
    raise AblyException(
        "Cannot use Basic Auth over non-TLS connections",
        401,
        40103)

It is compliant with both clauses and still fails this test, because the test requires the constructor itself to throw.

ably-pubsub-js did not derive the assertion at all: it reuses the id rest/unit/RSC18/basic-auth-over-http-rejected-1 for a test that constructs with token rather than a key and only checks the scheme (test/uts/rest/unit/rest_client.test.ts:197-214, expect(captured[0].url.protocol).to.equal('http:')) — i.e. it derived the section's trailing "Additional Test - Token auth over HTTP allowed" block and skipped the Basic-auth rejection.

Separately: the error code the test names, 40103, appears nowhere in features.md (grep -c 40103 → 0). The assertion's OR error.message CONTAINS … arms make that survivable, but the code is not spec-derived.

Proposal: rewrite the test so it accepts failure at either point — construct the client, then attempt a request, and assert that an error surfaces by the time the request would have gone out, with no request having reached the mock. That keeps the substance (private keys never leave over plaintext) and drops the unstated timing constraint. If constructor-time failure is intended to be normative, that belongs in RSC18 rather than only in the test.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions