Add display to the engine tests #16050
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changelog[uncommitted] (2024-05-10) |
tgummerer
force-pushed
the
tg/display-tests
branch
2 times, most recently
from
b35a02b
to
5fc7c92
Compare
tgummerer
added
the
impact/no-changelog-required
tgummerer
force-pushed
the
tg/display-tests
branch
3 times, most recently
from
e856c07
to
988e557
Compare
justinvp
approved these changes
May 9, 2024
| @@ -4535,7 +4548,8 @@ func TestResourceNames(t *testing.T) { | |||
| }) | |||
| hostF := deploytest.NewPluginHostF(nil, nil, programF, loaders...) | |||
| p := &TestPlan{ | |||
| Options: TestUpdateOptions{HostF: hostF}, | |||
| // Displaytests for this are racy | |||
There was a problem hiding this comment.
What's this comment for? Was this previously skipped?
There was a problem hiding this comment.
Yeah, I missed cleaning this up. I had started excluding tests from displaytests before I realized we need to do some sorting in the display code to get consistent output.
| return nil | ||
| } | ||
|
|
||
| // TODO: dedup with diff_test.go |
There was a problem hiding this comment.
In general, any TODO we have in the code should have an associated tracking issue and be linked to from here.
There was a problem hiding this comment.
Thanks for calling this out! It's actually easy enough to just address it rather than leaving a TODO around, so I just did that.
There was a problem hiding this comment.
I gave that a try and it ends up with an annoying circular dependency that's not easy to fix. Given that, and the fact that this is only used in two places, I don't think it's really worth the effort of factoring it out at this point, so I just removed the TODO.
| accept := cmdutil.IsTruthy(os.Getenv("PULUMI_ACCEPT")) | ||
| if !accept { | ||
| var err error | ||
| expectedStdout, err = os.ReadFile(path + "/diff.stdout.txt") |
There was a problem hiding this comment.
Should this use filepath.Join (applies throughout)?
Suggested change
| expectedStdout, err = os.ReadFile(path + "/diff.stdout.txt") | |
| expectedStdout, err = os.ReadFile(filepath.Join(path, "diff.stdout.txt")) |
There was a problem hiding this comment.
It definitely should, fixing that now.
tgummerer
force-pushed
the
tg/display-tests
branch
from
e6932b6
to
e685cea
Compare
tgummerer
force-pushed
the
tg/display-tests
branch
from
e685cea
to
5849258
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 9, 2024
We want to add more test coverage to the display code. The best way to do that is to add it to the engine tests, that already cover most of the pulumi functionality. It's probably not really possible to review all of the output, but at least it gives us a baseline, which we can work with. There's a couple of tests that are flaky for reasons I don't quite understand yet. I marked them as to skip and we can look at them later. I'd rather get in the baseline tests sooner, rather than spending a bunch of time looking at that. The output differences also seem very minor, so not super concerning. The biggest remaining issue is that this doesn't interact well with the Chdir we're doing in the engine. We could either pass the CWD through, or just try to get rid of that Chdir. So this should only be merged after #15607. I've tried to split this into a few commits, separating out adding the testdata, so it's hopefully a little easier to review, even though the PR is still quite large. One other thing to note is that we're comparing that the output has all the same lines, and not that it is exactly the same. Because of how the engine is implemented, there's a bunch of race conditions otherwise, that would make us have to skip a bunch of tests, just because e.g. resource A is sometimes deleted before resource B and sometimes it's the other way around. The biggest downside of that is that running with `PULUMI_ACCEPT` will produce a diff even when there are no changes. Hopefully we won't have to run that way too often though, so it might not be a huge issue? --------- Co-authored-by: Fraser Waters <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
tgummerer
force-pushed
the
tg/display-tests
branch
from
cbb3647
to
fe79ee0
Compare
tgummerer
force-pushed
the
tg/display-tests
branch
2 times, most recently
from
41081e2
to
9fb95dd
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
tgummerer
force-pushed
the
tg/display-tests
branch
from
9fb95dd
to
74c281e
Compare
tgummerer
force-pushed
the
tg/display-tests
branch
from
74c281e
to
574d0b5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We want to add more test coverage to the display code. The best way to do that is to add it to the engine tests, that already cover most of the pulumi functionality.
It's probably not really possible to review all of the output, but at least it gives us a baseline, which we can work with.
There's a couple of tests that are flaky for reasons I don't quite understand yet. I marked them as to skip and we can look at them later. I'd rather get in the baseline tests sooner, rather than spending a bunch of time looking at that. The output differences also seem very minor, so not super concerning.
The biggest remaining issue is that this doesn't interact well with the Chdir we're doing in the engine. We could either pass the CWD through, or just try to get rid of that Chdir. So this should only be merged after #15607.
I've tried to split this into a few commits, separating out adding the testdata, so it's hopefully a little easier to review, even though the PR is still quite large.
One other thing to note is that we're comparing that the output has all the same lines, and not that it is exactly the same. Because of how the engine is implemented, there's a bunch of race conditions otherwise, that would make us have to skip a bunch of tests, just because e.g. resource A is sometimes deleted before resource B and sometimes it's the other way around.
The biggest downside of that is that running with
PULUMI_ACCEPTwill produce a diff even when there are no changes. Hopefully we won't have to run that way too often though, so it might not be a huge issue?