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

chore: enable implicit default logger only in testing with -v #2877

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

apstndb
Copy link
Contributor

@apstndb apstndb commented Nov 6, 2024

What does this PR do?

This PR changes -v and -test.v=true handling only takes effect if testing.Testing() = true, i.e. go test.

Why is it important?

The current implementation always treats -v and -test.v=true specially, even outside of go test, and outputs verbose logs that are not easy to suppress. This makes it difficult to use testcontainers outside of go test.

Related issues

How to test this PR

These behaviors should not be changed.

$ go test
$ go test -v
$ go test -test.v=true

These behavior will be changed to suppress default logging.

$ go run ./ -v

@apstndb apstndb requested a review from a team as a code owner November 6, 2024 10:08
Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 629a514
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6746a72490587a00081a5b4d
😎 Deploy Preview https://deploy-preview-2877--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@apstndb apstndb changed the title Enable default logger only in testing Enable default logger only in testing with -v Nov 6, 2024
@mdelapenya
Copy link
Member

@apstndb thanks for opening the issue and working on the fix. I have one question about your use case: What do you expect when go run -v is executed?

From what I can see with the implementation in this PR, for the use case above of executing go run -v, we are going to always suppress the logging. I'd expect the -v flag enables the logging.

Could you share more on your use case so we can understand better the needs behind it? 🙏

@apstndb
Copy link
Contributor Author

apstndb commented Nov 6, 2024

My expectation is that go run ./(or binary built by go build) won't generally follow go test convention.

  • It won't know -test.v=true.
  • It can use --verbose or -vv for vebose mode, and it can even use -v for other purpose.

It means the current behavior of enforcing go test convention to go run ./ is not good.

In this case, users can implement their own handling by explicitly setting testcontainers.Logger = logger or testcontainers.WithLogger(logger). It is not hard.

@apstndb apstndb changed the title Enable default logger only in testing with -v Enable implicit default logger only in testing with -v Nov 7, 2024
@mdelapenya
Copy link
Member

@apstndb so IIUC, you are using testcontainers-go outside a test suite as, my guess, the runtime of a software that spins up containers, right?

Then I think you're right and we should consider this, but want to double check with you about your use case.

@apstndb
Copy link
Contributor Author

apstndb commented Nov 19, 2024

so IIUC, you are using testcontainers-go outside a test suite as, my guess, the runtime of a software that spins up containers, right?

Yes!
This is a combination of interactive tools and the Spanner emulator, so you could call it testing in the broadest sense, but it has nothing to do with the go test framework or unit testing.

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

@stevenh and I are in conversations to refactor the library logger.

In the mean time, @stevenh, do you think we can make progress and merge this one without affecting our goals? I think this change is fair simple, so harmless to merge.

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdelapenya mdelapenya self-assigned this Nov 20, 2024
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Nov 20, 2024
@mdelapenya mdelapenya changed the title Enable implicit default logger only in testing with -v chore: enable implicit default logger only in testing with -v Nov 20, 2024
@mdelapenya
Copy link
Member

@apstndb one final question: in your case, using tc-go as part of another library, how would you get testcontainers logs? IIUC this code, it will enable the verbose mode only when testing is enabled, which is not your case, right?

@apstndb
Copy link
Contributor Author

apstndb commented Nov 28, 2024

IIUC this code, it will enable the verbose mode only when testing is enabled, which is not your case, right?

It is right, my case don't show tc-go logs currently.

in your case, using tc-go as part of another library, how would you get testcontainers logs?

I think we can pass some options to the wrapper library like EnableLogs() or LogOutput(writer) explicitly.
IFAIK, some logs ("Relevant log output" of issue #2874 ) rely on default logger and not configurable using container logger. I think it shouldn't be.

@mdelapenya mdelapenya merged commit 512e503 into testcontainers:main Dec 5, 2024
122 checks passed
@mdelapenya
Copy link
Member

Merged, thanks for your work!

mdelapenya added a commit to mtellis2/testcontainers-go that referenced this pull request Dec 11, 2024
* main: (234 commits)
  chore(ci): add Github labels based on PR title (testcontainers#2914)
  chore(gha): Use official setup-docker-action (testcontainers#2913)
  chore(ci): enforce conventional commits syntax in PR titles (testcontainers#2911)
  feat(nats): WithConfigFile - pass a configuration file to nats server (testcontainers#2905)
  chore: enable implicit default logger only in testing with -v (testcontainers#2877)
  fix: container binds syntax (testcontainers#2899)
  refactor(cockroachdb): to use request driven options (testcontainers#2883)
  chore(deps): bump actions/setup-go from 5.0.0 to 5.1.0 (testcontainers#2904)
  chore(deps): bump ossf/scorecard-action from 2.3.1 to 2.4.0 (testcontainers#2903)
  chore(deps): bump test-summary/action from 2.3 to 2.4 (testcontainers#2902)
  feat(wait): strategy walk (testcontainers#2895)
  feat(wait): tls strategy (testcontainers#2896)
  docs: better contribution guidelines (testcontainers#2893)
  fix(influxdb): Respect custom waitStrategy (testcontainers#2845)
  fix: only upload to sonar on ubuntu-latest (testcontainers#2891)
  fix: build artifact name properly (testcontainers#2890)
  fix: do not run sonar upload when ryuk is disabled (testcontainers#2889)
  fix: update GH actions for uploading/downloading artifacts (testcontainers#2888)
  feat(ci): Enable master moby with rootless (testcontainers#2880)
  fix(redpanda): temporary file use
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Dec 13, 2024
* main:
  feat(gcloud)!: add support to seed data when using RunBigQueryContainer (testcontainers#2523)
  security(deps): bump golang.org/x/crypto from 0.28.0 to 0.31.0 (testcontainers#2916)
  chore(ci): add Github labels based on PR title (testcontainers#2914)
  chore(gha): Use official setup-docker-action (testcontainers#2913)
  chore(ci): enforce conventional commits syntax in PR titles (testcontainers#2911)
  feat(nats): WithConfigFile - pass a configuration file to nats server (testcontainers#2905)
  chore: enable implicit default logger only in testing with -v (testcontainers#2877)
  fix: container binds syntax (testcontainers#2899)
  refactor(cockroachdb): to use request driven options (testcontainers#2883)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: -v enable logs which is not intuitive to control
2 participants