[NO-REF] Handle non-JSON HTTP error bodies, vendor test fixtures, run tests serially - #266

Five commits. The first is a real SDK fix; the rest are about making this repo's test runs legible. One test still fails — details at the bottom, it needs someone with context on the test index.

1. Preserve HTTP error details when the response body is not JSON

helpers.throwHttpErrorFromResponse called response.json() on every non-2XX body. Error responses are not always JSON — rate limit and gateway responses are commonly plain text. For those the parse rejects and the SDK surfaces the parse failure rather than the request failure:

SyntaxError: Unexpected token 'T', "Too many requests" is not valid JSON
  at Response.json (node_modules/node-fetch/src/body.js:149:15)

The status, statusText, url and headers go with it — the details the function exists to attach. A caller cannot tell a rate limit from a bad gateway from a malformed payload. Now the body is read as text and parsed if possible, falling back to the raw text, and to HTTP <status> when empty. All modules route non-2XX responses through this one helper. tracker.js already handled non-JSON bodies this way when emitting error events.

Behavior change: error.message is now always a string. Previously a JSON body without a message field left it undefined, which makes error.message.toLowerCase() throw — spec/src/modules/catalog/catalog-groups-v2.js:223 does exactly that.

2. Vendor the catalog test fixtures

tasks.js and catalog-files.js downloaded fixtures from raw.githubusercontent.com on every run, before any assertion. Now committed under spec/fixtures/ (1.9 KB, byte-identical to the remote copies) and read from disk. spec/ is outside files in package.json, so nothing extra ships.

3-4. Run the suite serially

Every spec throttles itself with a 300ms afterEach, commented as being there to avoid rate limiting. One request per 300ms is ~200/min, just under the x-ratelimit-limit: 201 the API reports — the throttle is calibrated to the limit. --parallel gives each worker its own 300ms throttle, so N workers make N times that rate and the run exceeds the quota. That is why failures moved between spec files run to run.

Measured on this branch:

Commit 4 also reverts a 15s timeout I tried in commit 3: it made things worse, since a rate limited request holding a slot for 15s prolongs the pressure. .mocharc.json is back at 5s.

Trade-off: serial costs wall clock time. If that is not acceptable, the alternative is raising the rate limit on the test API key so --parallel fits within it.

5. Surface the real failure in one search test

spec/src/modules/search.js:465 had no rejection handler on its promise chain, so a failing assertion never reached done() and was reported as a timeout. It exceeded the 5s and the 15s timeout identically — the duration was never the problem. With .catch(done) the real error appears.

Worth noting the file has 53 .then((res) => { and 2 .catch(done), so other tests there can mask failures the same way. Left alone here.

The remaining failure — needs your context

1063 passing (4m)
1 failing

1) ConstructorIO - Search
     getSearchResults
       Should return a response with a valid query, section and preFilterExpression:
   AssertionError: expected 1 to deeply equal 2
   at spec/src/modules/search.js:493:51

The preFilterExpression is (group_id=BrandXY AND Color=red) OR (Color=blue AND Brand=XYZ). In items.csv, 10005 (BrandXY, red) matches the first branch and 10008 (blue, XYZ) the second, so the expected 2 looks right against the fixture. The index returns 1, which suggests the test index does not currently hold what the fixture describes — several specs call replaceCatalog against the shared test key, so its state is not fixture-determined.

I did not guess at the expected number. This one needs someone who knows the intended state of the test index.

🤖 Generated with Claude Code