#3 Implement additional LDClient tests. #36

kinyoklion · 2022-08-17T21:03:08Z

This PR adds top level tests for the LDClientImpl as well as LDClientNode.

Some tests have a certain amount of duplication between the two to ensure that the platform level tests are covered.

The tests are largely ports of the tests from the node SDK.

…implement-ldclient-tests

…update processors.

…en handles.

…r levels, but we need to check the event emitter.

…pper.

shortcut-integration · 2022-08-17T21:03:11Z

This pull request has been linked to Shortcut Story #162616: Implement LDClient unit tests..

…-from-testing

…t-ldclient-tests

kinyoklion · 2022-08-17T21:42:34Z

platform-node/package.json

    "ts-jest": "^27.1.4",
-    "typescript": "^4.6.3"
+    "typescript": "^4.6.3",
+    "launchdarkly-js-test-helpers": "^2.2.0"


The node level tests can benefit from this helper. For now I have excluded it from common, and server-common, because those tests should execute outside node without many polyfills or dependencies if we need them to. We may be able to more widely use them later.

kinyoklion · 2022-08-17T21:43:08Z

platform-node/__tests__/LDClientNode.bigSegments.test.ts

@@ -0,0 +1,151 @@
+import { integrations, interfaces, LDBigSegmentsOptions, LDLogger } from '@launchdarkly/js-server-sdk-common';


There are some node level big segments test for the event emitter implementation. The main testing is done in the common sdk.

kinyoklion · 2022-08-17T21:43:31Z

platform-node/__tests__/LDClientNode.fileDataSource.test.ts

@@ -0,0 +1,164 @@
+import { integrations } from '@launchdarkly/js-server-sdk-common';


Most of the file data source testing is at the common level, but here we make sure it works with the actual node filesystem APIs.

If it's just the I/O implementation that's distinctive here, I feel like you might be able to make the tests a bit simpler by omitting things like the segment. I mean, the fact that there's a flag referencing a segment is a detail of the data that's in the file, which should be orthogonal to how we read the file or detected changes to the file.

kinyoklion · 2022-08-17T21:43:47Z

platform-node/__tests__/LDClientNode.listeners.test.ts

@@ -0,0 +1,93 @@
+import { integrations } from '@launchdarkly/js-server-sdk-common';


Primarily for the event emitter connection.

kinyoklion · 2022-08-17T21:45:21Z

platform-node/__tests__/platform/NodeInfo.test.ts

@@ -1,5 +1,5 @@
 import * as os from 'os';


Moved to follow directory structure.

kinyoklion · 2022-08-17T21:45:31Z

platform-node/__tests__/platform/NodeRequests.test.ts

@@ -1,6 +1,6 @@
 import * as http from 'http';


Moved to follow directory structure.

eli-darkly

Looks good - I left a couple of comments about optional changes to consider.

eli-darkly · 2022-08-23T02:18:41Z

platform-node/__tests__/LDClientNode.fileDataSource.test.ts

@@ -0,0 +1,164 @@
+import { integrations } from '@launchdarkly/js-server-sdk-common';


If it's just the I/O implementation that's distinctive here, I feel like you might be able to make the tests a bit simpler by omitting things like the segment. I mean, the fact that there's a flag referencing a segment is a detail of the data that's in the file, which should be orthogonal to how we read the file or detected changes to the file.

eli-darkly · 2022-08-23T02:56:05Z

server-sdk-common/__tests__/LDClient.allFlags.test.ts

+      () => { },
+      () => { },
+      // Always listen to events.
+      () => true,


Something that's occurred to me during this reviews in general is that the large number of LDClientImpl constructor parameters makes it a bit hard to see which one is which. Might you consider creating a type whose only purpose is to make some of these into named properties? Like, the first three parameters are mandatory and generally self-explanatory at the call site, but these other callbacks not so much. If the usage were something like this—

new LDClientImpl( sdkKey, platform, config, { hasEventListeners: () => true });

—it'd be easier to see what's happening, and possibly harder to make a mistake by putting parameters in the wrong order. In this example I've assumed that there is some default behavior for the unspecified ones, which might not make sense in a non-test context since (I think) all of the SDK implementations are likely to need to provide all of those callbacks, but even if they're all mandatory & therefore can't make the test code less verbose, I feel like there might be some benefit.

I've implemented this change.

It does remove some verbosity in tests, because I added a makeCallbacks(withEvents:boolean) helper that most tests can use.

eli-darkly · 2022-08-23T03:02:14Z

server-sdk-common/__tests__/LDClient.events.test.ts

+      targets: [],
+      rules: [
+        {
+          clauses: [{ attribute: 'key', op: 'in', values: [defaultUser.key] }],


Just a thought for future test writing: something I've done in other SDKs, and wish I had gotten around to in Node, is to rely more heavily on helper functions for generating boilerplate pieces of flag configuration like this. For instance, if the idea is to make a clause that will definitely match this user, I would change this to something like clauses: [clauseThatMatchesUser(defaultUser)] and define clauseThatMatchesUser to be equivalent to this. Even though this kind of logic isn't rocket science and isn't incredibly verbose, I find that it's easier for me to understand and validate the test logic at a glance when there are fewer extraneous details and I can see the intention immediately from the names of things, rather than having to infer it from my knowledge of how such a clause would be evaluated.

kinyoklion added 30 commits July 29, 2022 14:25

Lint

4b51907

Write unit tests for file data source

f208ff4

Remove docs folder.

ca829ca

Merge main.

67fdde3

Merge branch 'main' into rlamb/sc-154357/file-data-source

f0a5080

Start adding top level client tests.

8daa0bb

Progress on tests.

7835b32

Start implementation of LDClientContext.

1b96fad

Merge branch 'rlamb/sc-154357/file-data-source' into rlamb/sc-162616/…

171ebd4

…implement-ldclient-tests

Change file data source factory to be LDClientContext based.

18590c4

Fix open handles in tests.

bbc4b9f

Update organization to allow for LDDataSourceUpdates to be used with …

5986d0b

…update processors.

First pass DataSourceUpdates.

d8b9565

Add DataSourceUpdates tests.

5bfc3da

Add client level individual eval tests.

b4b69ce

Progress on events.

01ca674

Remove upsert clone because we need to retain prototypes.

32e6eba

Add more events tests.

8c5ee0b

Start big segments tests.

516df56

Finish big segments test. Linting.

d427d1c

Ensure we close the big segments manager. Updated tests to prevent op…

2164dc3

…en handles.

Remove commented jest spy.

57c6a9f

Linting cleanup.

c08491b

File data source tests that use the node filesystem implementation.

76ec9a3

Add test for node level big segments. Most things are covered by lowe…

de3313b

…r levels, but we need to check the event emitter.

Fix TLS parsing.

4d23a6d

Add TLS tests.

7c38752

Lint

f929aee

Move tests to match directory structure. Add tests for the header wra…

8618392

…pper.

Merge main.

3f09fbc

kinyoklion added 4 commits August 17, 2022 13:31

Fixes issues from end to end testing.

ea0d8a2

merge main

8f25e12

Merge fixes branch.

454311b

Remove the logging destination from node level tests.

eeb064e

kinyoklion added 4 commits August 17, 2022 14:15

Merge main,

94a25e2

Diagnostic events error handling.

22661fe

Merge branch 'rlamb/diagnostic-event-error-handling' into rlamb/fixes…

39a0eea

…-from-testing

Merge branch 'rlamb/fixes-from-testing' into rlamb/sc-162616/implemen…

f199f0a

…t-ldclient-tests

kinyoklion changed the title ~~Rlamb/sc 162616/implement ldclient tests~~ #3 Implement additional LDClient tests. Aug 17, 2022

kinyoklion commented Aug 17, 2022

View reviewed changes

kinyoklion marked this pull request as ready for review August 17, 2022 21:46

kinyoklion added 2 commits August 17, 2022 15:36

Add missing describe name.

53707d4

Linting

745ea4b

eli-darkly approved these changes Aug 23, 2022

View reviewed changes

Base automatically changed from rlamb/fixes-from-testing to main August 23, 2022 16:28

kinyoklion added 4 commits August 23, 2022 10:39

Change callbacks to be an object for LDClientImpl.

c005500

Merge

9a2b10e

Linting

86ad7d3

Merge main.

2720d7a

kinyoklion merged commit 1f2bba7 into main Aug 23, 2022

kinyoklion deleted the rlamb/sc-162616/implement-ldclient-tests branch August 23, 2022 17:47

		@@ -0,0 +1,151 @@
		import { integrations, interfaces, LDBigSegmentsOptions, LDLogger } from '@launchdarkly/js-server-sdk-common';

		@@ -0,0 +1,164 @@
		import { integrations } from '@launchdarkly/js-server-sdk-common';

		@@ -0,0 +1,93 @@
		import { integrations } from '@launchdarkly/js-server-sdk-common';

#3 Implement additional LDClient tests. #36

#3 Implement additional LDClient tests. #36

Uh oh!

Conversation

kinyoklion commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shortcut-integration bot commented Aug 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eli-darkly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kinyoklion commented Aug 17, 2022 •

edited

Loading