Improve coverage and testability of config.ts #2288
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
src/config_test.ts
Outdated
| it('should try to load from WSL on Windows with wsl.exe not working', () => { | ||
| const kc = new KubeConfig(); | ||
| const commands: { command: string; args: string[] }[] = []; | ||
| (kc as any).spawnSync = (cmd: string, args: string[]) => { |
There was a problem hiding this comment.
No need to change this PR, but just something for future consideration - all supported Node release lines do have this type of mocking and spying built in.
There was a problem hiding this comment.
Thanks for pointing this out, it didn't show up when I looked around for mocking child_process but now I found it and I refactored the test to use this instead.
There was a problem hiding this comment.
Hm, I'm actually surprised that the CI is passing. Mocking an ES module like that should not work because they have immutable bindings. When I pull this PR down locally, these tests fail.
The approach here would work with the previous state of the PR though, where you were mocking the KubeConfig object's method. I think we should go back to that.
ESM mocking is quite the can of worms. There is support for that in newer versions of Node (and some ecosystem modules as well), but they build on top of Node's ESM loaders, which have been extremely unstable and are still experimental.
There was a problem hiding this comment.
I dug into this a bit more. The mock is surprisingly working. The reason the tests are failing on my local machine is that loadFromDefault() is returning early here, so the mocked spawnSync() calls never happen.
There was a problem hiding this comment.
This gets the new tests passing for me locally:
diff --git a/src/config_test.ts b/src/config_test.ts
index 9bb31dd5..30f4d49f 100644
--- a/src/config_test.ts
+++ b/src/config_test.ts
@@ -1613,6 +1613,17 @@ describe('KubeConfig', () => {
});
describe('BufferOrFile', () => {
+ let originalEnv;
+
+ before(() => {
+ originalEnv = process.env;
+ process.env = {};
+ });
+
+ after(() => {
+ process.env = originalEnv;
+ });
+
it('should load from root if present', () => {
const data = 'some data for file';
const arg: any = {Looking at the code, mocking fs.accessSync() to throw might also get the job done.
|
Comments addressed, please re-check. |
| @@ -322,6 +326,7 @@ describe('KubeConfig', () => { | |||
| }; | |||
|
|
|||
| assertRequestOptionsEqual(opts, expectedOptions); | |||
| strictEqual((requestContext.getAgent()! as any).options.servername, 'kube.example2.com'); | |||
There was a problem hiding this comment.
Isn't this rather an https.Agent than an any?
This would remove the unnecessary type casting but you probably still need the bang-operator or a different check.
Not sure if it's worth it, just wanted to mention that.
There was a problem hiding this comment.
options isn't exposed as public (at least not in the typings), so Typescript doesn't like it. The cast as any is mostly to get the typescript compiler to leave it alone.
|
/lgtm I wasn't ware of this form of mocking either, looks very cool! |
|
Opps - sorry, I wasn't aware that |
The tests in kubernetes-client#2288 can fail depending on the environment variables on the system. This commit takes the environment out of the equation. Refs: kubernetes-client#2288
The tests in kubernetes-client#2288 can fail depending on the environment variables on the system. This commit takes the environment out of the equation. Refs: kubernetes-client#2288
No description provided.