fix: add additional client tests and fixes #8
Conversation
| return { ...this.user, user_properties: userPropertiesCopy }; | ||
| } else { | ||
| return { ...this.user }; |
There was a problem hiding this comment.
Does user_properties change anything?
It's valid to spread a null or undefined variable, you just won't get any properties. So the before and after here are mostly functionally the same.
There was a problem hiding this comment.
This was added to make testing easier. Comparing the original user (null/undefined user_properties) and the one as a result of get/setUser()would not be equal.
| } | ||
| } | ||
|
|
||
| const API_KEY = 'client-DvWljIjiiuqLbyjqdvBaLFfEBrAvGuA3'; |
There was a problem hiding this comment.
These are tests against the live API?
There was a problem hiding this comment.
Yes. I could mock the response eventually, but its a lot easier to hit the live endpoint.
| const client = new ExperimentClient(API_KEY, { | ||
| fetchTimeoutMillis: 1, | ||
| }); | ||
| await client.fetch(testUser); |
There was a problem hiding this comment.
Does this fetch wait from the retry?
There was a problem hiding this comment.
No, the retries occur in the background.
There was a problem hiding this comment.
Could you assert that variant('sdk-ci-test') at this point returns the config fallback? I think that would help demonstrate the intention of the test.
packages/browser/test/client.test.ts
Outdated
| expect(variant).toEqual({ value: 'on', payload: 'payload' }); | ||
| }); | ||
|
|
||
| test('ExperimentClient.fetch, fallbacks returned in correct order', async () => { |
There was a problem hiding this comment.
In the context of the SDK I have no idea what this is testing.
I would like to see a clear description of the logic being tested step by step in any test with multiple steps.
On top of that this seems like it might be testing the API as much as the SDK?
We should have end-to-end tests somewhere but I'm not sure this suite is the right place for that. Probably a broader discussion to have outside of this PR.
There was a problem hiding this comment.
This test is mostly testing the order/priority of fallbacks within the SDK--both before and after fetching variants from the server.
I will add comments on each step describing what we are testing and what we expect.
|
@ManThursday could you re-review? Added a comment for each test case & split up the big one. |
trashstack
left a comment
trashstack
left a comment
There was a problem hiding this comment.
Breaking up the big test definitely helped. The descriptions are good as well although I had actually been thinking more along the lines of smaller comments annotating individual steps within the tests
| }); | ||
| let variant = client.variant(serverKey); | ||
| expect(variant).toEqual(serverOffVariant); | ||
| await client.fetch(testUser); |
There was a problem hiding this comment.
Is this just to populate local storage?
| let variant = client.variant(serverKey); | ||
| expect(variant).toEqual(serverOffVariant); |
There was a problem hiding this comment.
serverKey and serverOffVariant were initially quite confusing names for me, particularly in this test.
Have I got these right?
serverKey = An experiment key that does actually exist on the server.
serverOffVariant = The configured initial variant for serverKey which is not the same as any variant on the server for this experiment.
Summary
Checklist