[Testing Framework] Add test structure to views package for rendering test output #33324
Conversation
liamcervante
force-pushed
the
liamcervante/tests/view
branch
from
2dcb188
to
4e373dd
Compare
| } | ||
| } | ||
|
|
||
| if suite.Status <= moduletest.Skip { |
There was a problem hiding this comment.
It feels a little odd to use inequalities against state constants. I normally don't think of iota-derived constants as having an order. Did you consider using suite.Status == moduletest.Pending || suite.Status == moduletest.Skip and reject it?
There was a problem hiding this comment.
I can definitely update it to that here, but there's another function that also relies on the ordering. I added a comment there to highlight it.
Do you think it's worth me dropping any ordering assumptions to the status and updating the other function as well? I do think the iota implementation does enforce this, but I don't feel strongly about it.
There was a problem hiding this comment.
Nope, I hadn't spotted that and I think it's fine to rely on the ordering here. I'd maybe suggest adding commentary next to the type definition to clarify that order is significant and point to Merge as an example of that.
internal/moduletest/status.go
Outdated
|
|
||
| // ColorizedText returns textual representations of the status, but colored for | ||
| // visual appeal. | ||
| func (status Status) ColorizedText(color *colorstring.Colorize) string { |
There was a problem hiding this comment.
This seems like view logic which I'd expect to be in the corresponding view package. Would it cause problems to move it over? If you're concerned about forgetting to add future status values we could add the views package to the scripts/exhaustive.sh list.
| // | ||
| // The implementation basically always returns the highest of the two, which | ||
| // means the order the statuses are defined within the iota matters. | ||
| func (status Status) Merge(next Status) Status { |
There was a problem hiding this comment.
@alisdair, this function does make an assumption on the ordering, basically to simplify the logic of combining multiple status. For example a test that has one error case, one fail case and two pass cases would report overall as error. So the ordering is deliberate in order to make this function simple.
| } | ||
| } | ||
|
|
||
| if suite.Status <= moduletest.Skip { |
There was a problem hiding this comment.
I can definitely update it to that here, but there's another function that also relies on the ordering. I added a comment there to highlight it.
Do you think it's worth me dropping any ordering assumptions to the status and updating the other function as well? I do think the iota implementation does enforce this, but I don't feel strongly about it.
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
This PR introduces a views/test.go file which adds a human-readable view for test outputs. A JSON view will be added in a later PR.
In addition, representations of test suites, test files, and test runs are added. These will be provided by the testing framework into the view to render.
Target Release
1.6.0
Terraform Testing Framework
This PR is part of a chain of PRs implementing the new Terraform test command.
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.