-
Notifications
You must be signed in to change notification settings - Fork 1
JSON e2e test #28
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
JSON e2e test #28
Conversation
e17e954 to
a27b368
Compare
|
|
Why are we deleting the YamlBlankLinesCompletion test? |
| position: { line: 127, character: 15 }, | ||
| expectation: CompletionExpectationBuilder.create() | ||
| .expectContainsItems(['Fn::FindInMap']) | ||
| .todo() //Autocomplete looks for Fn or ! in YAML for Intrinsic Function |
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.
yaml?
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.
Ignore the YAML that was for my understanding. The important part is that when looking for an intrinsic function in JSON the router expects "Fn:" to set the shouldUseIntrinsicFunctionProvider to true (CompletionRouter.ts, line 236). The "Fi" doesn't go through the if block.
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.
I can try to add a fix to make this more function more robust.
| position: { line: 127, character: 69 }, | ||
| expectation: CompletionExpectationBuilder.create() | ||
| .expectContainsItems(['InstanceType']) | ||
| .todo() //returns nothing in valid JSON |
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.
really? does this match functional testing observation?
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.
I haven't checked it in functional testing, but I think the issue is that, getSecondLevelKeyCompletions (IntrinsicFunctionArgumentProvider.ts, line 512) returns undefined. The current line in the JSON looks like:
{"Fn::FindInMap": ["RegionMap", {"Ref": "AWS::Region"}, "I"]}
The second argument is an object at parse time so it goes through this if block:
if (!Array.isArray(args) || args.length < 2 || typeof args[0] !== 'string' || typeof args[1] !== 'string') {
log.debug('Invalid arguments for second-level key completions');
return undefined;
}
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.
I messed around with the code and found another issue with getSecondLevelKeyCompletions:
Even if after this line
{"Fn::FindInMap": ["RegionMap", {"Ref": "AWS::Region"}, "I"]}
We read in the dynamic topLevelKey as valid, the code will return undefined at this check:
const secondLevelKeys = mappingEntity.getSecondLevelKeys(topLevelKey);
if (secondLevelKeys.length === 0) {
console.log(`No second-level keys found for mapping: ${mappingName}, top-level key: ${topLevelKey}`);
return undefined;
}
getSecondLevelKeys expects a string parameter, so passing in the reference will return [] with length 0.
| position: { line: 477, character: 14 }, | ||
| expectation: CompletionExpectationBuilder.create() | ||
| .expectItems(['Value']) | ||
| .todo() //works in real template not in e2e |
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.
that should never happen. see if we can figure out the inconsistency
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.
This is also what happens in YAML. The comment is actually just a copy of the comment for this step in the YAML test. Will look into both cases.
| @@ -1,270 +0,0 @@ | |||
| import { describe, it } from 'vitest'; | |||
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.
Why was this test removed?
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.
This was a mistake, was not supposed to be in the commit
| position: { line: 580, character: 79 }, | ||
| expectation: CompletionExpectationBuilder.create() | ||
| .expectItems(['AMI']) | ||
| .todo() //Returns nothing for both JSON and YAML |
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.
It would be better to add a parameter to the todo function that explains why the todo is there, instead of using inline comments to explain the reason for todo
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.
agreed
* Update Entity provider to only suggest on keys
Filled in the autocomplete test for a JSON document. Test failing