Skip to content

Conversation

@kinyoklion
Copy link
Member

This primarily adds a test for using a proxy.

It also uncovered a problem with the import of the library for making a proxy, so that is fixed.

…r levels, but we need to check the event emitter.
@kinyoklion kinyoklion changed the title Add tests for using a proxy. #4 Add tests for using a proxy. Aug 17, 2022
@@ -1,4 +1,5 @@
import createHttpsProxyAgent, { HttpsProxyAgentOptions } from 'https-proxy-agent';
import * as createHttpsProxyAgent from 'https-proxy-agent';
import { HttpsProxyAgentOptions } from 'https-proxy-agent';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I only care about the .d.ts, so this import is fine.

@@ -1,4 +1,5 @@
import createHttpsProxyAgent, { HttpsProxyAgentOptions } from 'https-proxy-agent';
import * as createHttpsProxyAgent from 'https-proxy-agent';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typings are odd here. Importing this way will give me the correct behavior. The typings were clearly happy with the other way, but runtime was not. (The downside of typescript being that it cannot really tell what is going on in the non-typescript parts all the time.)

on: false,
offVariation: 0,
variations: [expectedFlagValue, 'no'],
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another kind of situation where I would favor using a helper to construct the flag data. I mean, if I understand correctly, all that matters is that we want the flag to return expectedFlagValue at all times; the literal data here is just boilerplate to accomplish that.

};
const allData = { flags: { flagKey: flag }, segments: {} };

describe('', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty string in describe feels a bit odd to me. I mean, if these are all tests for being able to use a proxy, wouldn't that be a logical group description?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a mistake. I will fix that.

Base automatically changed from rlamb/sc-162616/implement-ldclient-tests to main August 23, 2022 17:47
@kinyoklion kinyoklion merged commit 7274899 into main Aug 23, 2022
@kinyoklion kinyoklion deleted the rlamb/proxy-tests branch August 23, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants