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

[Testing Framework] Add test file HCL configuration and parser functionality #33325

Merged
merged 4 commits into from
Jun 22, 2023

Conversation

liamcervante
Copy link
Member

@liamcervante liamcervante commented Jun 7, 2023

This PR adds the HCL representation of our testing files into the configs package.

It includes updates to the parsing library with the introduction of a set of parallel *WithTests functions that load any testing files alongside the requested root module. New functions were introduced (instead of expanding existing functions) to ensure that import operations (such as plan or apply) aren't blocked by syntax errors in or partial implementations of testing files. This matches the behaviour of the Go programming languages, where go build main.go will not load any testing files. In a later PR, the terraform validate command will be updated to load and also validate the testing files but for the time being only terraform test will load and use the new testing files.

This PR also includes the ability to customise the testing directory that contains the test files. This will eventually be hooked up to the test command itself.

Target Release

1.6.0

Terraform Testing Framework

This PR is part of a chain of PRs implementing the new Terraform test command.

Description Branch PR
Cleanup previous implementation liamcervante/tests/cleanup #33323
Add initial human view implementation liamcervante/tests/view #33324
Add Terraform test file config support liamcervante/tests/config #33325
Update terraform context functionality liamcervante/tests/context #33326
Add test command to Terraform CLI liamcervante/tests/command #33327

The above PRs implement the core functionality of the new test command. More PRs will follow implementing specific features, but the test command will work end-to-end once the above PRs are merged.

These changes are implemented in a chain, with each change building on the previous one to ensure ease of review. To see the combined changes as one, navigate to the last PR in the chain and change it to merge directly into main.

Base automatically changed from liamcervante/tests/view to main June 13, 2023 08:09
@@ -88,7 +119,9 @@ func (p *Parser) loadFiles(paths []string, override bool) ([]*File, hcl.Diagnost
return files, diags
}

func (p *Parser) dirFiles(dir string) (primary, override []string, diags hcl.Diagnostics) {
func (p *Parser) dirFiles(dir string, testsDir string) (primary, override, tests []string, diags hcl.Diagnostics) {
Copy link
Member

Choose a reason for hiding this comment

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

It took me a while to understand what this function is doing now. While I think the previous function signature was basically self-explanatory, a function called dirFiles which takes two arguments isn't quite as intuitive. Perhaps a comment would help.

As a starting point, here's my understanding, which might of course be wrong:

dirFiles finds Terraform configuration file paths which are direct descendants of dir, splitting them into primary files and overrides based on filename. If testsDir is non-empty and a sub-directory of that name exists, dirFiles also finds test file paths in that sub-directory and returns them separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment!

@@ -111,6 +168,13 @@ func (p *Parser) dirFiles(dir string) (primary, override []string, diags hcl.Dia
continue
}

if ext == ".tftest" || ext == ".tftest.json" {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we handle .tftest.json here, but not in fileExt below?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a bug, fixed and added a test that loads .tftest.json files to verify the fix.

}

if attr, exists := content.Attributes["command"]; exists {
expr, exprDiags := shimTraversalInString(attr.Expr, true)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using shimTraversalInString here (and for "mode") instead of using the expression directly? I thought this was just for legacy syntax compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I liked the functionality of shimTraversalInString in that it allowed users to also specify the keyword with quotation marks without crashing, so I reused it. I didn't realise it was just for older parts of the config, and have now read the doc comment more thoroughly.

I've removed referencing to it and I'm just using the expression directly now.

@liamcervante liamcervante merged commit cad9aa9 into main Jun 22, 2023
@liamcervante liamcervante deleted the liamcervante/tests/config branch June 22, 2023 15:03
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants