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

Terraform Syntax Testing and Target Syntax Generation #2

Closed
wants to merge 8 commits into from

Conversation

jpogran
Copy link
Contributor

@jpogran jpogran commented Jan 11, 2022

  • update to latest terraform grammer from extension
  • testing infrastructure
  • build target syntaxes

@jpogran jpogran changed the title syntaxes Terraform Syntax Testing and Target Syntax Generation Jan 11, 2022
@jpogran jpogran self-assigned this Jan 12, 2022
@jpogran jpogran added the enhancement New feature or request label Jan 12, 2022
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

A few questions/suggestions:

I'm all for supporting more formats, but I think that we should also be aware of why we support any particular format. Do you mind adding a short list of all formats (cson, JSON, YAML, PLIST) and at least one example to each to a Readme? - Just to make sure it's a conscious decision to commit to supporting a format - i.e. we all know why we're doing this and will have something to refer to in the future.

Feel free to lookup ENG-030 - I did some research on this and left some notes with links there.

I hate to say this (after you have done all the work) 😅 but seeing it now all in action I would argue that the tests written in the caret comments syntax in https://github.com/PanAeon/vscode-tmgrammar-test would be a little easier to maintain, as one doesn't have to deal with index numbering and the test failures also seem easier to read/fix as you can directly see the piece of config being tested in the failure. Also it is written in TypeScript and uses the same textmate vscode library under the hood. That said I may not have the same context as you do as I have never written such tests before.

Comment on lines +36 to +37
const tokens = result.tokens;
return tokens;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const tokens = result.tokens;
return tokens;
return result.tokens;

Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Thank you for all the work of putting this together!

I like the structure of the jest tests, but I'm also worried that having to fiddle with many indexes might hurt our DX in the future. I wondered if having a clever test helper would make things easier, but I'm unsure if something like this would be flexible enough for all scenarios.

expect(tokens.slice(0, 11)).toMatchScopes([
  'entity.name.type.terraform',

  'punctuation.definition.string.begin.terraform',
  'string.quoted.double.terraform',
  'punctuation.definition.string.end.terraform',

  'punctuation.definition.string.begin.terraform',
  'string.quoted.double.terraform',
  'punctuation.definition.string.end.terraform',
  
  'punctuation.section.block.begin.terraform',
]);

This doesn't account for tokens' length or exact position (yet).

@dbanck
Copy link
Member

dbanck commented Jan 17, 2022

Another possible way is to make use of Jest snapshot tests. For example, like this:

expect(tokens.slice(0, 11)).toMatchInlineSnapshot(`
Array [
  Object {
    "endIndex": 8,
    "scopes": Array [
      "source.terraform",
      "meta.block.terraform",
      "entity.name.type.terraform",
    ],
    "startIndex": 0,
  },
  Object {
    "endIndex": 9,
    "scopes": Array [
      "source.terraform",
      "meta.block.terraform",
    ],
    "startIndex": 8,
  },
  Object {
    "endIndex": 10,
    "scopes": Array [
      "source.terraform",
      "meta.block.terraform",
      "string.quoted.double.terraform",
      "punctuation.definition.string.begin.terraform",
    ],
    "startIndex": 9,
  },
  ...
]
`);

This avoids typing object structures by hand and will verify correctness once generated but isn't easy to read.

@dbanck
Copy link
Member

dbanck commented Jan 19, 2022

Like you mentioned in Slack, having vscode-tmgrammer-test generate snapshot tests seems like the best of both worlds. You avoid tedious writing and still get a more readable result than the Jest tests.

The snapshots seem a bit bloated, though. There are a lot of comment.terraform entries. We should make sure that the source file for the snapshot test only contains Terraform code and not any grammar test comments.

@jpogran
Copy link
Contributor Author

jpogran commented Feb 1, 2022

Closing in favor of #3

@jpogran jpogran closed this Feb 1, 2022
@jpogran jpogran deleted the syntaxes branch March 4, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants