-
Notifications
You must be signed in to change notification settings - Fork 1
🧪 [testing improvement] Add tests for JulesClient constructor #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0060670
77197d0
18b1090
f3afcb0
04919d9
86b4ecd
cd689b5
f35c503
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,66 @@ | ||||||
| import { JulesClient } from '../src/client'; | ||||||
| import assert from 'node:assert'; | ||||||
| import { test } from 'node:test'; | ||||||
| import fs from 'node:fs'; | ||||||
| import os from 'node:os'; | ||||||
|
|
||||||
| test('JulesClient constructor uses provided API key', () => { | ||||||
| const apiKey = 'test-api-key'; | ||||||
| const client = new JulesClient(apiKey); | ||||||
| assert.strictEqual((client as any).apiKey, apiKey); | ||||||
| }); | ||||||
|
|
||||||
| test('JulesClient constructor uses API key from environment if not provided', (t) => { | ||||||
| const originalEnvKey = process.env.JULES_API_KEY; | ||||||
| process.env.JULES_API_KEY = 'env-api-key'; | ||||||
| t.after(() => { | ||||||
| process.env.JULES_API_KEY = originalEnvKey; | ||||||
| }); | ||||||
|
|
||||||
| const client = new JulesClient(); | ||||||
| assert.strictEqual((client as any).apiKey, 'env-api-key'); | ||||||
| }); | ||||||
GreyC marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+1
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current tests cover providing the API key directly and via an environment variable, which is great. However, to ensure full coverage of the
This will make the test suite more robust and prevent future regressions related to API key handling. import { JulesClient } from '../src/client';
import assert from 'node:assert';
import { test } from 'node:test';
import * as fs from 'fs';
test('JulesClient constructor uses provided API key', () => {
const apiKey = 'test-api-key';
const client = new JulesClient(apiKey);
assert.strictEqual((client as any).apiKey, apiKey);
});
test('JulesClient constructor uses API key from environment if not provided', (t) => {
const originalEnvKey = process.env.JULES_API_KEY;
process.env.JULES_API_KEY = 'env-api-key';
t.after(() => {
process.env.JULES_API_KEY = originalEnvKey;
});
const client = new JulesClient();
assert.strictEqual((client as any).apiKey, 'env-api-key');
});
test('JulesClient constructor uses API key from config file', (t) => {
const originalEnvKey = process.env.JULES_API_KEY;
delete process.env.JULES_API_KEY;
t.after(() => {
process.env.JULES_API_KEY = originalEnvKey;
});
t.mock.method(fs, 'existsSync', () => true);
t.mock.method(fs, 'readFileSync', () => JSON.stringify({ apiKey: 'config-api-key' }));
const client = new JulesClient();
assert.strictEqual((client as any).apiKey, 'config-api-key');
});
test('JulesClient constructor throws when no API key is found', (t) => {
const originalEnvKey = process.env.JULES_API_KEY;
delete process.env.JULES_API_KEY;
t.after(() => {
process.env.JULES_API_KEY = originalEnvKey;
});
t.mock.method(fs, 'existsSync', () => false);
assert.throws(
() => {
new JulesClient();
},
{
message: 'No API key found. Set JULES_API_KEY or run: jules_cli setup',
}
);
});
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jules Please add the two missing test cases suggested here:
Do not add any temp directories. Only modify packages/cli/test/client.test.ts. |
||||||
|
|
||||||
| test('JulesClient constructor uses API key from config file if not provided via env', (t) => { | ||||||
| const originalEnvKey = process.env.JULES_API_KEY; | ||||||
| delete process.env.JULES_API_KEY; | ||||||
| t.after(() => { | ||||||
| process.env.JULES_API_KEY = originalEnvKey; | ||||||
| }); | ||||||
|
|
||||||
| const configPath = os.homedir() + '/.config/jules/config.json'; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For cross-platform compatibility, it's better to construct file paths using
Suggested change
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jules Please fix both this and the comment on line 57 (id:2934907534) together in one commit:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've implemented the requested path handling improvements in |
||||||
|
|
||||||
| t.mock.method(fs, 'existsSync', (path: string) => { | ||||||
| if (path === configPath) return true; | ||||||
| return false; | ||||||
| }); | ||||||
|
|
||||||
| t.mock.method(fs, 'readFileSync', (path: string) => { | ||||||
| if (path === configPath) { | ||||||
| return JSON.stringify({ apiKey: 'config-api-key' }); | ||||||
| } | ||||||
| throw new Error('File not found'); | ||||||
| }); | ||||||
|
|
||||||
| const client = new JulesClient(); | ||||||
| assert.strictEqual((client as any).apiKey, 'config-api-key'); | ||||||
| }); | ||||||
|
|
||||||
| test('JulesClient constructor throws error if no API key is found anywhere', (t) => { | ||||||
| const originalEnvKey = process.env.JULES_API_KEY; | ||||||
| delete process.env.JULES_API_KEY; | ||||||
| t.after(() => { | ||||||
| process.env.JULES_API_KEY = originalEnvKey; | ||||||
| }); | ||||||
|
|
||||||
| t.mock.method(fs, 'existsSync', (path: string) => { | ||||||
| if (path.includes('.config/jules/config.json')) return false; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Suggested change
|
||||||
| return true; // allow other existsSync calls to proceed normally if any | ||||||
| }); | ||||||
|
|
||||||
| assert.throws(() => { | ||||||
| new JulesClient(); | ||||||
| }, { | ||||||
| message: 'No API key found. Set JULES_API_KEY or run: jules_cli setup' | ||||||
| }); | ||||||
| }); | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR description mentions using
node --experimental-strip-types, which requires explicit file extensions in import paths. To ensure the tests can run in that environment, you should add the.tsextension to this import.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot add .ts extensions here — this was already tried and reverted in commits 77197d0 and 18b1090 because tsc errors with TS5097 ('An import path can only end with a .ts extension when allowImportingTsExtensions is enabled'). The project compiles with tsc without allowImportingTsExtensions, so .ts extensions in imports are not valid. The test runner used is node:test via tsc-compiled output, not --experimental-strip-types directly.