test cleanup #2
test cleanup #2
Conversation
|
A little unfortunate that your commit is based on master, that conflates a bunch of changes with your actual test change. Any chance you could rebase on @julsemaan's PR branch? |
| type TestSshServer struct { | ||
| Host string | ||
| Port int64 | ||
| } |
There was a problem hiding this comment.
I put this file in the testutil directory so it can be reused in the other test that depends on an SSH server.
I made that change here: theneva@bbae30e, and I'd love to include it as part of this PR. However, the test fails for me locally even on master, and I don't want to touch it (or block this PR on that unrelated change) since I can't verify the fix.
I'm happy to send that commit as a separate PR to pulumi/pulumi-command after this PR lands.
There was a problem hiding this comment.
That's a great move. I can help getting the other test to pass. It should pass on master in any case. What's the error you're getting?
There was a problem hiding this comment.
Just to be clear: The new test I wrote passes on my machine just fine. I hope we can land this PR without figuring out why the TestOptionalLogging test fails for me!
Buuuut since you asked:
I get assertions failures that seem to indicate that the output from the SSH server just doesn't get registered by the context.
I made a quick recording, showing me running go test ./... from ./provider on the latest origin/master (9144544):
command-test-error-on-master.mov
I'm running this Go (installed with homebrew):
$ go version
go version go1.22.3 darwin/arm64
… on this machine:
I also tried running go install ./... and go mod tidy just to be sure, but neither command did anything.
Again, I'm happy to help with figuring out why that test fails for me, but I'd really like to land this PR first 😅
There was a problem hiding this comment.
I can reproduce this and will look into it.
There was a problem hiding this comment.
The regression was introduced by 36798ab Upgrade to latest pulumi-go-provider v0.17 (#442).
The reason CI tests pass is that it runs go test in provider/tests, not /provider - needs to be fixed as well.
There was a problem hiding this comment.
I see! That sounds like a fix for master. I'm happy to submit the change to use the new common test SSH server once it works on master
There was a problem hiding this comment.
Awesome! By the way, are you able to merge this PR? If you can, I'm pretty sure Julien won't mind 😄
sorry @thomas11, I did change my mind and change it to be like you ask, but it looks like you're just too fast for me 😅 care to take another look now that the diff is smaller, please? |
provider/tests/provider_test.go
Outdated
| @@ -331,11 +313,12 @@ func TestRemoteCommandStdoutStderrFlag(t *testing.T) { | |||
| }) | |||
|
|
|||
| t.Run("update-actual-without-std", func(t *testing.T) { | |||
| t.Parallel() | |||
There was a problem hiding this comment.
Both of these tests update the same resource, right? Can they really run in any order then?
There was a problem hiding this comment.
Huh, I apparently only read half of your comment, and just made them run in parallel…
I didn't have any issues with the parallelism when running the tests locally, but I suppose you're right that they might get in each other's way.
Addressing your original comment on this: The create step does have an assertion that's relevant to the test, so I don't really view it as pure setup. Keeping it as part of the test – the way it is right now – makes the most sense to me.
Do you think this part looks ready to merge now?
| type TestSshServer struct { | ||
| Host string | ||
| Port int64 | ||
| } |
There was a problem hiding this comment.
That's a great move. I can help getting the other test to pass. It should pass on master in any case. What's the error you're getting?
|
@julsemaan looks like we're good to go! If you agree, will you merge this PR? 😄 |
|
I cleaned up the tests according to @thomas11's review