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.

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

add tests for terminal suggest widget, fix some bugs #234445

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Nov 22, 2024

Screenshot 2024-11-22 at 11 09 26 PM

@meganrogge meganrogge self-assigned this Nov 22, 2024
@meganrogge meganrogge added this to the November 2024 milestone Nov 22, 2024
@meganrogge meganrogge enabled auto-merge (squash) November 22, 2024 19:27
@meganrogge
Copy link
Contributor Author

Actually wip realized something

@meganrogge meganrogge marked this pull request as draft November 22, 2024 22:30
scripts/test-integration.sh Outdated Show resolved Hide resolved
});
});

function createTestCase(name: string, commandLineWithCursor: string, expectedSpecs: string[], resourcesRequested: 'files' | 'folders' | 'both' | 'neither', availableSpecs: Fig.Spec[], availableCommands: Set<string>): void {
Copy link
Member

Choose a reason for hiding this comment

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

If you move this inside suite you'll be able to access variables you declare in that block. This lets you do things like this:

let commands: string[]

setup(() => {
  commands = ['default', 'set', ...];
});

function assertCompletions() {
  // references commands
}

test(() => {
  commands = ['custom test commands'];
});

});

suite('prefix: c', () => {
createTestCase(`No available commands:`, 'c|', [], 'neither', availableSpecs, new Set());
Copy link
Member

Choose a reason for hiding this comment

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

I'd only generate tests inside a function like this if they originate from some array of specifications which is sometimes useful. Pulling the test call out will make integration with IDEs better and make them easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there will be quite a bit of duplication if I do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also not sure what you mean about integration with other IDEs

Copy link
Member

Choose a reason for hiding this comment

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

The current way you're missing out on VS Code integration at the test level:

image

Copy link
Member

@Tyriar Tyriar Nov 23, 2024

Choose a reason for hiding this comment

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

If you do want to keep it this way though, you should put all the test specifics into an array and create them that way:

Instead of:

createTestCase('|', availableCommands, 'neither', availableSpecs);
createTestCase('c|', availableCommands, 'neither', availableSpecs);
...

Do:

const testSpecs: [my, type, ...] = [
  ['|', availableCommands, 'neither', availableSpecs]
];
// or
const testSpecs = [
  { input: '|', expectedCompletionLabels: 'files' | 'folders' | 'both' | 'neither', resourcesRequested: 'neither', availableSpecs }
];

for (const spec of testSpecs) {
  test(testSpecs.name, () => {
    // use spec to implement the test
  });
}

This saves more of the repetitive writing. This is similar to terminal link tests:

suite('removeLinkSuffix', () => {
for (const testLink of testLinks) {
test('`' + testLink.link + '`', () => {
deepStrictEqual(
removeLinkSuffix(testLink.link),
testLink.suffix === undefined ? testLink.link : testLink.link.replace(testLink.suffix, '')
);
});
}
});

@meganrogge meganrogge marked this pull request as ready for review November 23, 2024 03:12
@meganrogge meganrogge changed the title add initial tests for suggest widget add initial tests for suggest widget, fix some bugs Nov 23, 2024
@meganrogge meganrogge changed the title add initial tests for suggest widget, fix some bugs add tests for terminal suggest widget, fix some bugs Nov 23, 2024
createTestCase('code-insiders --locale | && ls', ['bg', 'de', 'en', 'es', 'fr', 'hu', 'it', 'ja', 'ko', 'pt-br', 'ru', 'tr', 'zh-CN', 'zh-TW'], 'neither', availableSpecs);
});

function createTestCase(commandLineWithCursor: string, expectedCompletionLabels: string[], resourcesRequested: 'files' | 'folders' | 'both' | 'neither', availableSpecs: Fig.Spec[]): void {
Copy link
Member

Choose a reason for hiding this comment

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

Best to stick expected as the last arg

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.

2 participants