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

Fix test logging output. #473

Merged
merged 1 commit into from
Jun 11, 2020
Merged

Fix test logging output. #473

merged 1 commit into from
Jun 11, 2020

Conversation

paddycarver
Copy link
Contributor

In version 1.0.0-1.6.0, our in-process non-binary test driver didn't
call go-plugin, and called log.SetOutput to control what got logged, to
avoid flooding test output with debug logs.

From version 1.6.0, the in-process non-binary test driver had the same
behavior, but we added an out-of-process binary test driver, which used
go-plugin, but had the provider in a separate process that sent its
stdout and stderr through Terraform. Terraform then, not the test
framework, was responsible for filtering the output.

With the introduction of our in-process binary test driver, however,
we're using go-plugin and Terraform isn't filtering log output for us.
In theory, the log.SetOutput call that was working for the in-process
non-binary test driver should've worked here, as well, and it sort of
did. The issue was that go-plugin calls log.SetOutput and sets it to
os.Stderr in plugin.Serve, which overrides our filter.

To get this behavior back, after every call to plugin.Serve, we override
the go-plugin log.SetOutput by calling our own again.

This also removes the helper/resource.logOutput function, replacing its
usage with helper/logging.SetOutput, which does the same thing. The only
difference should be that filtering on test name is no longer supported.
If we want to support that, we would need to plumb a testing.T through
to runProviderCommand, which involves updating a lot of code for
marginal gain; as far as I know, nobody actually uses the test name
filtering for logging, preferring instead to log everything and only run
the test they want logs for.

In version 1.0.0-1.6.0, our in-process non-binary test driver didn't
call go-plugin, and called log.SetOutput to control what got logged, to
avoid flooding test output with debug logs.

From version 1.6.0, the in-process non-binary test driver had the same
behavior, but we added an out-of-process binary test driver, which used
go-plugin, but had the provider in a separate process that sent its
stdout and stderr through Terraform. Terraform then, not the test
framework, was responsible for filtering the output.

With the introduction of our in-process binary test driver, however,
we're using go-plugin and Terraform isn't filtering log output for us.
In theory, the log.SetOutput call that was working for the in-process
non-binary test driver should've worked here, as well, and it sort of
did. The issue was that go-plugin calls log.SetOutput and sets it to
os.Stderr in plugin.Serve, which overrides our filter.

To get this behavior back, after every call to plugin.Serve, we override
the go-plugin log.SetOutput by calling our own again.

This also removes the helper/resource.logOutput function, replacing its
usage with helper/logging.SetOutput, which does the same thing. The only
difference should be that filtering on test name is no longer supported.
If we want to support that, we would need to plumb a testing.T through
to runProviderCommand, which involves updating a lot of code for
marginal gain; as far as I know, nobody actually uses the test name
filtering for logging, preferring instead to log everything and only run
the test they want logs for.
@paddycarver paddycarver added bug Something isn't working testing labels Jun 11, 2020
@paddycarver paddycarver added this to the v2.0.0 milestone Jun 11, 2020
@paddycarver paddycarver requested review from kmoe and appilon June 11, 2020 13:12
@@ -451,49 +447,6 @@ type TestStep struct {
providers map[string]*schema.Provider
}

// Set to a file mask in sprintf format where %s is test name
const envLogPathMask = "TF_LOG_PATH_MASK"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe TF_LOG_PATH_MASK may be used in our acctests in TeamCity, is this accounted for in the new implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am investigating if this is something we can live without

@paddycarver paddycarver merged commit d609e41 into master Jun 11, 2020
@paddycarver paddycarver deleted the paddy_fix_test_logging branch June 11, 2020 18:07
@ghost
Copy link

ghost commented Jul 12, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants