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

fix: use virtual caret in tests #722

Merged
merged 1 commit into from
Jun 4, 2022

Conversation

grant-d
Copy link
Contributor

@grant-d grant-d commented Jun 2, 2022

What does this PR do?

It improves core test infrastructure:

While helping to fix several autocomplete bugs (eg #724) in the language server, it was difficult to understand where the "cursor" was placed in sample content within the tests.

The existing code uses the following method signature, whereby the caller has to do the math on the position parameter to establish where the caret is placed:

// Method
function parseSetup(content: string, position: number): ...

// Caller
const content = 'hello world this is great';
const completion = parseSetup(content, 10); // <---- Can you easily tell where the cursor is?
`

While this works, it is difficult to maintain since it's not easy to understand the test by just looking at the content (& without doing math)

This PR introduces the notion of a virtual caret, which is specified in the content itself.
The token |:| is chosen to be easily noticed, and not conflict with 'real' yaml

// Method
function parseSetup(content: string, position?: number): ... // position optional if caret specified

// Caller
const content = 'hello worl|:|d this is great'; // Note embedded caret token `|:|` representing cursor pos
const completion = parseSetup(content); // Zero calories
`

What issues does this PR fix or reference?

Tests are simpler to write and maintain since the cursor's position is explicit in the content.

Is it tested? How?

The changed method is back-compatible, so all existing tests continue to pass.
However, I have updated one example test of each interesting scenario:

  • When the caret is a prefix, eg |:|abc
  • When the caret is a postfix, eg abc|:|
  • When the caret is an infix, eg ab|:|c

If this PR is approved, I will update the other tests accordingly. I updated just a few tests for now to keep the noise down and the changes easy to identify.

cc @p-spacek

Comment on lines +45 to +48
if (typeof position === 'undefined') {
position = content.indexOf('|:|');
content = content.replace('|:|', '');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the critical change
If the position is not specified, we look inside the content to derive the position using the index of the |:| token

Comment on lines +75 to +76
const content = '|:|';
const completion = parseSetup(content);
Copy link
Contributor Author

@grant-d grant-d Jun 2, 2022

Choose a reason for hiding this comment

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

Scenario 1: position === 0

Comment on lines +99 to +100
const content = 'na|:|';
const completion = parseSetup(content);
Copy link
Contributor Author

@grant-d grant-d Jun 2, 2022

Choose a reason for hiding this comment

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

Scenario 2: position === <postfix>

Comment on lines +195 to +196
const content = 'nam|:|e';
const completion = await parseSetup(content);
Copy link
Contributor Author

@grant-d grant-d Jun 2, 2022

Choose a reason for hiding this comment

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

Scenario 3: position === <infix>

Comment on lines +561 to +562
const content = 'scripts: |:|';
const completion = parseSetup(content);
Copy link
Contributor Author

@grant-d grant-d Jun 2, 2022

Choose a reason for hiding this comment

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

Scenario 4: position === <length>

@@ -111,8 +111,9 @@ export class YamlCompletion {
setKubernetesParserOption(doc.documents, isKubernetes);

const offset = document.offsetAt(position);
const text = document.getText();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perf optimization: just call getText() once

@coveralls
Copy link

coveralls commented Jun 3, 2022

Coverage Status

Coverage increased (+0.003%) to 82.403% when pulling a8f2c11 on p-spacek:fix/caret into a3dd4e6 on redhat-developer:main.

@gorkem
Copy link
Collaborator

gorkem commented Jun 3, 2022

This is so cool. First on my list for review.

Copy link
Contributor

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the contribution

@msivasubramaniaan msivasubramaniaan merged commit 7fa3693 into redhat-developer:main Jun 4, 2022
@grant-d
Copy link
Contributor Author

grant-d commented Jun 4, 2022

Thanks @gorkem , @msivasubramaniaan

@grant-d grant-d deleted the fix/caret branch June 4, 2022 23:25
@p-spacek
Copy link
Contributor

p-spacek commented Jun 6, 2022

@grant-d love this, counting chars was an adventure :)
is it possible to add this also into autoCompletionFix.test.ts file?

@grant-d
Copy link
Contributor Author

grant-d commented Jun 6, 2022

Yes - my plan was to introduce the concept in a bite-sized PR, then once I got buy-in, I will spread the love elsewhere

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.

None yet

5 participants