Skip to content
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.

Sign up for GitHub

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

chore(cli-repl): use strict TS flag for test files #2149

Merged
merged 1 commit into from
Sep 4, 2024
Merged

chore(cli-repl): use strict TS flag for test files #2149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/cli-repl/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"compile": "tsc -p tsconfig.json",
"start": "node bin/mongosh.js",
"pretest": "npm run compile",
"test": "cross-env TS_NODE_PROJECT=../../configs/tsconfig-mongosh/tsconfig.test.json mocha -r \"../../scripts/import-expansions.js\" --timeout 60000 -r ts-node/register \"./src/**/*.spec.ts\"",
"test": "mocha -r \"../../scripts/import-expansions.js\" --timeout 60000 -r ts-node/register \"./src/**/*.spec.ts\"",
"test-ci": "node ../../scripts/run-if-package-requested.js npm test",
"test-coverage": "nyc --no-clean --cwd ../.. --reporter=none npm run test",
"test-ci-coverage": "nyc --no-clean --cwd ../.. --reporter=none npm run test-ci",
Expand Down
40 changes: 20 additions & 20 deletions packages/cli-repl/src/arg-parser.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,8 +705,8 @@ describe('arg-parser', function () {
});

it('sets the filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test1.js');
expect(parseCliArgs(argv).fileNames[1]).to.equal('test2.js');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test1.js');
expect(parseCliArgs(argv).fileNames?.[1]).to.equal('test2.js');
});
});

Expand All @@ -718,8 +718,8 @@ describe('arg-parser', function () {
});

it('sets the filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test1.mongodb');
expect(parseCliArgs(argv).fileNames[1]).to.equal('test2.mongodb');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test1.mongodb');
expect(parseCliArgs(argv).fileNames?.[1]).to.equal('test2.mongodb');
});
});

Expand All @@ -731,8 +731,8 @@ describe('arg-parser', function () {
});

it('sets the filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test1.txt');
expect(parseCliArgs(argv).fileNames[1]).to.equal('test2.txt');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test1.txt');
expect(parseCliArgs(argv).fileNames?.[1]).to.equal('test2.txt');
});
});

Expand All @@ -744,8 +744,8 @@ describe('arg-parser', function () {
});

it('sets the filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test1.txt');
expect(parseCliArgs(argv).fileNames[1]).to.equal('test2.txt');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test1.txt');
expect(parseCliArgs(argv).fileNames?.[1]).to.equal('test2.txt');
});
});

Expand All @@ -764,8 +764,8 @@ describe('arg-parser', function () {
});

it('sets the filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test1.txt');
expect(parseCliArgs(argv).fileNames[1]).to.equal('test2.txt');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test1.txt');
expect(parseCliArgs(argv).fileNames?.[1]).to.equal('test2.txt');
});
});
});
Expand All @@ -779,8 +779,8 @@ describe('arg-parser', function () {
});

it('sets the filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test1.js');
expect(parseCliArgs(argv).fileNames[1]).to.equal('test2.js');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test1.js');
expect(parseCliArgs(argv).fileNames?.[1]).to.equal('test2.js');
});
});

Expand All @@ -792,8 +792,8 @@ describe('arg-parser', function () {
});

it('sets the filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test1.mongodb');
expect(parseCliArgs(argv).fileNames[1]).to.equal('test2.mongodb');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test1.mongodb');
expect(parseCliArgs(argv).fileNames?.[1]).to.equal('test2.mongodb');
});
});

Expand All @@ -807,7 +807,7 @@ describe('arg-parser', function () {
});

it('uses the remainder as filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test2.txt');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test2.txt');
});
});

Expand All @@ -821,7 +821,7 @@ describe('arg-parser', function () {
});

it('uses the remainder as filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal('test2.txt');
expect(parseCliArgs(argv).fileNames?.[0]).to.equal('test2.txt');
});
});

Expand All @@ -842,7 +842,7 @@ describe('arg-parser', function () {
});

it('uses the remainder as filenames', function () {
expect(parseCliArgs(argv).fileNames[0]).to.equal(
expect(parseCliArgs(argv).fileNames?.[0]).to.equal(
'mongodb://domain.foo.js'
);
});
Expand Down Expand Up @@ -892,7 +892,7 @@ describe('arg-parser', function () {
});

context('when providing a deprecated argument', function () {
[
for (const { deprecated, replacement, value } of [
{ deprecated: 'ssl', replacement: 'tls' },
{
deprecated: 'sslAllowInvalidCertificates',
Expand Down Expand Up @@ -929,7 +929,7 @@ describe('arg-parser', function () {
replacement: 'tlsDisabledProtocols',
value: 'disabledProtos',
},
].forEach(({ deprecated, replacement, value }) => {
] as const) {
it(`replaces --${deprecated} with --${replacement}`, function () {
const argv = [...baseArgv, `--${deprecated}`];
if (value) {
Expand All @@ -940,7 +940,7 @@ describe('arg-parser', function () {
expect(args).to.not.have.property(deprecated);
expect(args[replacement]).to.equal(value ?? true);
});
});
}
});
});
});
4 changes: 2 additions & 2 deletions packages/cli-repl/src/async-repl.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,9 @@ describe('AsyncRepl', function () {
exited = true;
});

let resolve;
let resolve!: () => void;
repl.context.asyncFn = () =>
new Promise((res) => {
new Promise<void>((res) => {
resolve = res;
});

Expand Down
41 changes: 24 additions & 17 deletions packages/cli-repl/src/cli-repl.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ describe('CliRepl', function () {
const tmpdir = useTmpdir();

async function log(): Promise<any[]> {
if (!cliRepl.logWriter?.logFilePath) return [];
await cliRepl.logWriter.flush(); // Ensure any pending data is written first
return readReplLogfile(cliRepl.logWriter.logFilePath);
}
Expand All @@ -76,8 +77,8 @@ describe('CliRepl', function () {
});
exitCode = null;

let resolveExitPromise;
exitPromise = new Promise((resolve) => {
let resolveExitPromise!: () => void;
exitPromise = new Promise<void>((resolve) => {
resolveExitPromise = resolve;
});

Expand Down Expand Up @@ -858,7 +859,7 @@ describe('CliRepl', function () {
try {
await cliRepl.start('', {});
expect.fail('missed exception');
} catch (err) {
} catch (err: any) {
expect(err.message).to.equal('nested error');
}
});
Expand All @@ -869,7 +870,7 @@ describe('CliRepl', function () {
try {
await cliRepl.start('', {});
expect.fail('missed exception');
} catch (err) {
} catch (err: any) {
expect(err.message).to.equal(
'Cannot use --json without --eval or with --shell or with extra files'
);
Expand All @@ -884,7 +885,7 @@ describe('CliRepl', function () {
try {
await cliRepl.start('', {});
expect.fail('missed exception');
} catch (err) {
} catch (err: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to use strict (it doesn't need to happen on this PR) I would prefer to use a bit more strict typing here. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Errors have to be unknown or any because programming error (like trying to use some property of undefined) can also get there so you can never know what kind of error it would be. There's an argument for starting with unknown, though. But in tests any is really probably fine..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are going to use strict (it doesn't need to happen on this PR)

Well, that's what this PR currently does 🙂

I would prefer to use a bit more strict typing here. WDYT?

Could you give an example of what you have in mind here? Are you talking about this specific line or the PR in general? For catches, TS essentially only gives us a choice between unknown and any – we can use unknown, but it's honestly not super clear to me what the benefit of that by itself is in a test file.

If we want to ensure that we get an Error object here, we should be asserting that using chai regardless of the TS typings we're using, right? And I don't disagree with that, but that would apply to almost all catch {} blocks in tests in our codebases 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors have to be unknown or any because programming error (like trying to use some property of undefined) can also get there so you can never know what kind of error it would be

It's the simple fact that you can throw any value, not just any error, in particular things like throw null; work just fine in JS 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sorry, I meant that using strict and be more strict on typings.

I know that TypeScript has a limitation on specifying types on catch and I know the excuse from Microsoft 🧌, but probably we are expecting a really specific type of exception here so we don't want to pass the test if the error is a NPE or something else. So I would add a chai assertion making sure that the type is the one we want and fail otherwise (we were expecting exception 'A' but got 'B').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I would add a chai assertion making sure that the type is the one we want and fail otherwise (we were expecting exception 'A' but got 'B').

I'll put that on our tech alignment sync agenda, because this seems like a gigantic undertaking if we want to start doing it consistently across all our tests 🙂

expect(err.message).to.equal(
'Cannot use --json without --eval or with --shell or with extra files'
);
Expand All @@ -899,7 +900,7 @@ describe('CliRepl', function () {
try {
await cliRepl.start('', {});
expect.fail('missed exception');
} catch (err) {
} catch (err: any) {
expect(err.message).to.equal(
'Cannot use --json without --eval or with --shell or with extra files'
);
Expand Down Expand Up @@ -1285,7 +1286,7 @@ describe('CliRepl', function () {
it('does not emit warnings when connecting multiple times', async function () {
await cliRepl.start(await testServer.connectionString(), {});
let warnings = 0;
const warningListener = (warning) => {
const warningListener = (warning: unknown) => {
console.log('Unexpected warning', warning);
warnings++;
};
Expand Down Expand Up @@ -1407,7 +1408,9 @@ describe('CliRepl', function () {
input.write('exit\n');
await waitBus(cliRepl.bus, 'mongosh:closed');
const useEvents = requests.flatMap((req) =>
JSON.parse(req.body).batch.filter((entry) => entry.event === 'Use')
JSON.parse(req.body).batch.filter(
(entry: any) => entry.event === 'Use'
)
);
expect(useEvents).to.have.lengthOf(1);
});
Expand All @@ -1425,7 +1428,9 @@ describe('CliRepl', function () {
input.write('exit\n');
await waitBus(cliRepl.bus, 'mongosh:closed');
const useEvents = requests.flatMap((req) =>
JSON.parse(req.body).batch.filter((entry) => entry.event === 'Use')
JSON.parse(req.body).batch.filter(
(entry: any) => entry.event === 'Use'
)
);
expect(useEvents).to.have.lengthOf(0);
});
Expand All @@ -1448,7 +1453,9 @@ describe('CliRepl', function () {
input.write('exit\n');
await waitBus(cliRepl.bus, 'mongosh:closed');
const useEvents = requests.flatMap((req) =>
JSON.parse(req.body).batch.filter((entry) => entry.event === 'Use')
JSON.parse(req.body).batch.filter(
(entry: any) => entry.event === 'Use'
)
);
expect(useEvents).to.have.lengthOf(2);
});
Expand All @@ -1469,7 +1476,7 @@ describe('CliRepl', function () {
const loadEvents = requests
.map((req) =>
JSON.parse(req.body).batch.filter(
(entry) => entry.event === 'Script Loaded'
(entry: any) => entry.event === 'Script Loaded'
)
)
.flat();
Expand All @@ -1486,7 +1493,7 @@ describe('CliRepl', function () {
const apiEvents = requests
.map((req) =>
JSON.parse(req.body).batch.filter(
(entry) => entry.event === 'API Call'
(entry: any) => entry.event === 'API Call'
)
)
.flat();
Expand All @@ -1500,12 +1507,12 @@ describe('CliRepl', function () {

it('includes a statement about flushed telemetry in the log', async function () {
await cliRepl.start(await testServer.connectionString(), {});
const { logFilePath } = cliRepl.logWriter;
const { logFilePath } = cliRepl.logWriter!;
input.write('db.hello()\n');
input.write('exit\n');
await waitBus(cliRepl.bus, 'mongosh:closed');
const flushEntry = (await readReplLogfile(logFilePath)).find(
(entry) => entry.id === 1_000_000_045
(entry: any) => entry.id === 1_000_000_045
);
expect(flushEntry.attr.flushError).to.equal(null);
expect(flushEntry.attr.flushDuration).to.be.a('number');
Expand All @@ -1522,7 +1529,7 @@ describe('CliRepl', function () {
expect(
requests
.flatMap((req) =>
JSON.parse(req.body).batch.map((entry) => entry.event)
JSON.parse(req.body).batch.map((entry: any) => entry.event)
)
.sort()
.filter(Boolean)
Expand Down Expand Up @@ -1550,7 +1557,7 @@ describe('CliRepl', function () {
const apiEvents = requests
.map((req) =>
JSON.parse(req.body).batch.filter(
(entry) => entry.event === 'API Call'
(entry: any) => entry.event === 'API Call'
)
)
.flat();
Expand Down Expand Up @@ -1712,7 +1719,7 @@ describe('CliRepl', function () {

const connectEvents = requests.flatMap((req) =>
JSON.parse(req.body).batch.filter(
(entry) => entry.event === 'New Connection'
(entry: any) => entry.event === 'New Connection'
)
);
expect(connectEvents).to.have.lengthOf(1);
Expand Down
2 changes: 1 addition & 1 deletion packages/cli-repl/src/format-output.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ test 558.79 GiB
function () {
for (const type of ['ExplainOutput', 'ExplainableCursor']) {
it(`returns output with large depth (${type})`, function () {
const value = {};
const value: Record<string, unknown> = {};
let it = value;
for (let i = 0; i <= 20; i++) {
it = it[`level${i}`] = {};
Expand Down
27 changes: 17 additions & 10 deletions packages/cli-repl/src/mongosh-repl.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ describe('MongoshNodeRepl', function () {
return 'success';
});
cp.listConfigOptions.callsFake(() => Object.keys(config));
cp.exit.callsFake(((code) => bus.emit('test-exit-event', code)) as any);
cp.exit.callsFake(((code: number) =>
bus.emit('test-exit-event', code)) as any);

ioProvider = cp;

Expand All @@ -91,8 +92,8 @@ describe('MongoshNodeRepl', function () {
serviceProvider = sp;
calledServiceProviderFunctions = () =>
Object.fromEntries(
Object.keys(sp)
.map((key) => [key, sp[key]?.callCount])
Object.entries(sp)
.map(([key, value]) => [key, value?.callCount])
.filter(([, count]) => !!count)
);

Expand All @@ -105,7 +106,7 @@ describe('MongoshNodeRepl', function () {
mongoshRepl = new MongoshNodeRepl(mongoshReplOptions);
});

let originalEnvVars;
let originalEnvVars: Record<string, string | undefined>;
before(function () {
originalEnvVars = { ...process.env };
});
Expand Down Expand Up @@ -527,7 +528,9 @@ describe('MongoshNodeRepl', function () {
let getHistory: () => string[];

beforeEach(function () {
const { history } = mongoshRepl.runtimeState().repl as any;
const { history } = mongoshRepl.runtimeState().repl as unknown as {
history: string[];
};
getHistory = () =>
history.filter((line) => !line.startsWith('prefill-'));
for (let i = 0; i < prefill; i++) {
Expand Down Expand Up @@ -827,10 +830,12 @@ describe('MongoshNodeRepl', function () {
});

it('does not refresh the prompt if a window resize occurs while evaluating', async function () {
let resolveInProgress;
mongoshRepl.runtimeState().context.inProgress = new Promise((resolve) => {
resolveInProgress = resolve;
});
let resolveInProgress!: () => void;
mongoshRepl.runtimeState().context.inProgress = new Promise<void>(
(resolve) => {
resolveInProgress = resolve;
}
);
input.write('inProgress\n');
await tick();

Expand Down Expand Up @@ -933,7 +938,9 @@ describe('MongoshNodeRepl', function () {
context('user prompts', function () {
beforeEach(function () {
// No boolean equivalent for 'passwordPrompt' in the API, so provide one:
mongoshRepl.runtimeState().context.booleanPrompt = (question) => {
mongoshRepl.runtimeState().context.booleanPrompt = (
question: string
) => {
return Object.assign(mongoshRepl.onPrompt(question, 'yesno'), {
[Symbol.for('@@mongosh.syntheticPromise')]: true,
});
Expand Down
2 changes: 1 addition & 1 deletion packages/cli-repl/src/tls-certificate-selector.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ describe('arg-mapper.applyTlsCertificateSelector', function () {
);
exportCertificateAndPrivateKey = sinon.stub();
require(process.env.TEST_OS_EXPORT_CERTIFICATE_AND_KEY_PATH).setFn(
(search) => exportCertificateAndPrivateKey(search)
(search: unknown) => exportCertificateAndPrivateKey(search)
);
});
afterEach(function () {
Expand Down
Loading
Loading