test: Implement shared tests from spec submodule#47
Conversation
There was a problem hiding this comment.
This is pointed to an old version of the specifications submodule as we have not yet implemented fractional rollout to segments.
| return new Promise((resolve) => { | ||
| this.server = http.createServer((req, res) => { | ||
| // URL will be updated to /api/toggles/evaluations/v3/ in a later PR | ||
| if (req.method === "GET" && req.url === "/api/featuretoggles/v3/") { |
There was a problem hiding this comment.
URL will be updated to /api/toggles/evaluations/v3/ when we switch over to the new endpoint.
f6b1ca6 to
fdc5b79
Compare
There was a problem hiding this comment.
Pull request overview
Adds fixture-driven “specification” tests by pulling fixtures from a git submodule and running them against the provider via a lightweight local HTTP server.
Changes:
- Added a
specificationgit submodule to source shared JSON fixtures. - Added a test harness (
Server) plus a Jest suite to run all fixture cases againstOctopusFeatureProvider. - Updated CI workflow checkout to include submodules so fixtures are available in CI.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/specificationTests/server.ts |
Introduces a local HTTP server used by fixture-based tests. |
src/specificationTests/fixtureEvaluationTests.test.ts |
Loads JSON fixtures from the submodule and runs them as parameterized Jest tests. |
.gitmodules |
Adds the specification submodule definition. |
.github/workflows/build-test-release.yml |
Ensures CI checks out submodules for the test job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4fbe6df to
5a97a43
Compare
liamhughes
left a comment
There was a problem hiding this comment.
Looking good!
A few optional improvement questions, plus one about localStorage.
| import { Server } from "./server"; | ||
|
|
||
| interface Fixture { | ||
| response: V1FeatureToggleEvaluation[]; |
There was a problem hiding this comment.
Can declare response as unknown given we don't actually use any of its fields.
There was a problem hiding this comment.
Makes sense 🙂
| for (const file of fixtureFiles) { | ||
| const json = fs.readFileSync(path.join(fixturesDir, file), "utf-8"); | ||
| const fixture: Fixture = JSON.parse(json); | ||
| const testResponse = JSON.stringify(fixture.response); |
There was a problem hiding this comment.
In the other libraries we are avoiding deserialising and then reserialising the response. It probably matters less in JavaScript/TypeScript given the objects aren't strongly typed, but it might be nice to read response as a raw string if it's not too tricky/hacky?
There was a problem hiding this comment.
I've had a look at how we might do this and it's possible but I think it would be overkill considering that parsing and stringifying the JSON should be lossless in TS.
There was a problem hiding this comment.
Can add a comment explaining the difference between this and the .NET/Java implementations 👍
| }); | ||
|
|
||
| // Requires `specification` submodule to be initialized. | ||
| test.each(testCases)("$testCase.description", async ({ testResponse, testCase }) => { |
There was a problem hiding this comment.
Could we include the source JSON filename in the test name? Makes it easier to track where cases are configured.
There was a problem hiding this comment.
Good point! But I think there are pros and cons to this 😊 Having the file name in the test name could make it easier to find the file, but we already have the case description in test name, and that's easy to paste into a search box.
If we added the file name to the test names, it might actually make it harder to search on the description as we wouldn't be able to just bung the whole test name into a search box to find the right case.
There was a problem hiding this comment.
Okay, may need to put it to the vote when Dylan's back; I added them in the Java implementation. 😅
| }); | ||
|
|
||
| beforeEach(() => { | ||
| localStorage.clear(); |
There was a problem hiding this comment.
I'm not sure why we are referencing localStorage here?
There was a problem hiding this comment.
If I'm understanding this correctly, we need to clear local storage between test runs because we are caching the manifest when we call getEvaluationContext, and we don't want this to stick around between tests.
There was a problem hiding this comment.
Ahh, okay. Feels a bit like the abstraction is leaking, and the tests are fine without it. The cache only appears to be used if the HTTP client is unable to reach the OctoToggle service (see getFeatureManifest).
There was a problem hiding this comment.
I'm happy to take it out if it's not needed 👍
liamhughes
left a comment
There was a problem hiding this comment.
Approved, but if it were me, I'd remove the localStorage reset.
Background 🌇
As per this ADR, we are adding a shared testing fixture to ensure consistency across all of our OpenFeature implementations.
Each test case contains a description, associated toggle slug, evaluation context, and expected outcome, as well as a list of toggles in the shape of
V1FeatureToggleEvaluationto evaluate against.What's this? 🌵
This PR adds a test which stands up a fake HTTP server which serves a list of toggles (keyed by a client id generated in each test) then runs each case as a separate test.
The shared data is accessed as a Git submodule from our specifications repository. The current code is pointing to an older version of the test fixtures as we are yet to implement functionality (fractional segment rollout) already covered by the latest test fixtures.
How to review? 🔍
☑️ Check for consistency with .NET and Java implementations
Completes DEVEX-139