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

Reduce Output in Atmos Tests #923

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Reduce Output in Atmos Tests #923

wants to merge 27 commits into from

Conversation

Cerebrovinny
Copy link
Collaborator

@Cerebrovinny Cerebrovinny commented Jan 9, 2025

what

  • Introduced verbosity levels. Allows customizable detail.
  • Updated GitHub Actions: Modified test execution. Added verbosity control.
  • Improved Logging: Validated logs-level flag. Ensures correct log levels.
  • Updated README: Documented test verbosity. Guides on usage.

why

  • Reduces redundant output.
  • CI/CD: Provides clearer feedback

references

@mergify mergify bot added the triage Needs triage label Jan 9, 2025
@osterman osterman added the no-release Do not create a new release (wait for additional code changes) label Jan 10, 2025
Copy link

mergify bot commented Jan 10, 2025

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label Jan 10, 2025
@mergify mergify bot removed needs-cloudposse Needs Cloud Posse assistance triage Needs triage labels Jan 10, 2025
@Cerebrovinny Cerebrovinny reopened this Jan 10, 2025
Copy link

mergify bot commented Jan 10, 2025

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label Jan 10, 2025
@Cerebrovinny Cerebrovinny force-pushed the DEV-2909 branch 3 times, most recently from 414bb4a to 28e8c98 Compare January 13, 2025 23:26
@Cerebrovinny Cerebrovinny marked this pull request as ready for review January 14, 2025 11:47
@Cerebrovinny Cerebrovinny requested review from a team as code owners January 14, 2025 11:47
.github/workflows/test.yml Outdated Show resolved Hide resolved
tests/cli_test.go Outdated Show resolved Hide resolved
@Cerebrovinny Cerebrovinny requested a review from aknysh January 14, 2025 12:59
Makefile Outdated Show resolved Hide resolved
@Cerebrovinny Cerebrovinny requested a review from osterman January 16, 2025 12:31
README.md Outdated Show resolved Hide resolved
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 16, 2025
@aknysh
Copy link
Member

aknysh commented Jan 18, 2025

@Cerebrovinny please resolve the conflicts
Does this PR have unresolved conversations or comments that you think need to be addressed?

Copy link

mergify bot commented Jan 20, 2025

💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Jan 20, 2025
@mergify mergify bot removed the conflict This PR has conflicts label Jan 20, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 21, 2025
@osterman
Copy link
Member

osterman commented Jan 21, 2025

@Cerebrovinny I’ve been reflecting on this, and I think my ideal implementation would suppress all logs unless there’s a failure. In the case of a failure, it would then emit the full log. Is that how it works?

@Cerebrovinny
Copy link
Collaborator Author

Cerebrovinny commented Jan 23, 2025

@Cerebrovinny I’ve been reflecting on this, and I think my ideal implementation would suppress all logs unless there’s a failure. In the case of a failure, it would then emit the full log. Is that how it works?

The quiet mode shows minimal output and only failures
But it's not buffering logs either - it's just filtering what gets printed

see here
https://github.com/cloudposse/atmos/actions/runs/12897213551/job/35962047152?pr=923#step:11:1

@osterman
Copy link
Member

@Cerebrovinny I’ve been reflecting on this, and I think my ideal implementation would suppress all logs unless there’s a failure. In the case of a failure, it would then emit the full log. Is that how it works?

The quiet mode shows minimal output and only failures But it's not buffering logs either - it's just filtering what gets printed

see here https://github.com/cloudposse/atmos/actions/runs/12897213551/job/35962047152?pr=923#step:11:1

Can you show me what a failure looks like? Is it the same, but just says failed? If so, that won't fly.

Would it be possible to buffer the logs? I don't think we can use quiet mode in CI, b/c it will make supporting open source contributions too cumbersome if the maintainers have to check out the branch, just to see what the failure/error was.

@@ -135,3 +135,27 @@ introduction: |-
Find all documentation at: [atmos.tools](https://atmos.tools)

contributors: []

testing:
Copy link
Member

Choose a reason for hiding this comment

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

Is testing a supported section in the README.md.tmpl?

Copy link

mergify bot commented Jan 24, 2025

💥 This pull request now has conflicts. Could you fix it @Cerebrovinny? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Jan 24, 2025
@osterman
Copy link
Member

@Cerebrovinny I would prefer a different approach that doesn’t involve wrapping existing functions and introducing a new lexicon for testing.

Ideal requirements:

  • non need to wrap individual functions with feature flags to silence output
  • just work for anything that produces output.
  • buffer output and only emit when there’s any error are errors
  • Feature flags (env) or argument, that disables buffering, but it should default to enabled

Pseudo code (gpt):


package mypackage

import (
	"bytes"
	"fmt"
	"os"
	"testing"
)

func captureOutput(fn func()) (string, error) {
	// Save the original os.Stdout and os.Stderr
	origStdout := os.Stdout
	origStderr := os.Stderr

	// Create pipes to capture output
	rOut, wOut, err := os.Pipe()
	if err != nil {
		return "", err
	}
	rErr, wErr, err := os.Pipe()
	if err != nil {
		return "", err
	}

	// Redirect os.Stdout and os.Stderr to the pipes
	os.Stdout = wOut
	os.Stderr = wErr

	// Create a channel to capture the output
	outputChan := make(chan string)
	go func() {
		var buf bytes.Buffer
		buf.ReadFrom(rOut)
		buf.ReadFrom(rErr)
		outputChan <- buf.String()
	}()

	// Run the provided function
	fn()

	// Restore the original os.Stdout and os.Stderr
	wOut.Close()
	wErr.Close()
	os.Stdout = origStdout
	os.Stderr = origStderr

	// Get the captured output
	output := <-outputChan
	return output, nil
}

func TestExample(t *testing.T) {
	// Wrap the test logic in the output capture
	output, err := captureOutput(func() {
		// Simulate commands or deeply nested code that outputs
		fmt.Println("This is stdout")
		fmt.Fprintln(os.Stderr, "This is stderr")
		fmt.Println("Another stdout message")
	})
	if err != nil {
		t.Fatalf("Failed to capture output: %v", err)
	}

	// Example assertion
	if false { // Simulate a test failure condition
		t.Errorf("Test failed. Output:\n%s", output)
	}
}

@mergify mergify bot removed the conflict This PR has conflicts label Jan 24, 2025
@@ -135,3 +135,27 @@ introduction: |-
Find all documentation at: [atmos.tools](https://atmos.tools)

contributors: []

testing:
content: |-
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a misunderstanding how the README.yaml works. We run this though gomplate and apply this template: https://github.com/cloudposse/.github/blob/main/README.md.gotmpl

There's no section we look for called content or testing.

Also, after updating the README.yaml, run make readme to rebuild it.

Note, we're going to do this with atmos soon, see:

And the confusion here, shows we should have a JSON schema.

@@ -14,6 +14,8 @@ import (
"github.com/cloudposse/atmos/pkg/schema"
)

// captureOutput is a simple stdout capture utility specific to logger tests.
// For more comprehensive output capture in other tests, use the testhelper package.
func captureOutput(f func()) string {
r, w, _ := os.Pipe()
stdout := os.Stdout
Copy link
Member

Choose a reason for hiding this comment

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

We also need to capture stderr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cloudposse Needs Cloud Posse assistance no-release Do not create a new release (wait for additional code changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants