Increasing coverage and simplifying coverage report creation. #3103
Conversation
This file contains hidden or 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
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR aims to increase unit and integration test coverage while simplifying the creation of coverage reports. Key changes include:
- Adding new test cases for Flink CLI error scenarios (empty cloud provider and empty region).
- Updating golden files for error output messages.
- Enhancing tests for login commands and BYOK functionality.
- Modifying the Makefile to improve test execution and coverage data merging.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/flink_test.go | New tests for detecting empty cloud provider and region scenarios |
| test/fixtures/output/flink/region/unset-region.golden | Added golden file for region unset command |
| test/fixtures/output/flink/endpoint/* | New golden files for handling missing cloud provider, region, or empty args |
| internal/login/command_test.go | Added test for suspended organization error handling |
| internal/byok/command_create_test.go | Expanded table-driven testing for GCP metadata and BYOK policy command generation |
| Makefile | Updated targets for unit, integration tests, and coverage data merging |
Comments suppressed due to low confidence (1)
test/flink_test.go:609
- The fixture name specified in TestFlinkEmptyRegion ('flink/region/unset.golden') does not match the actual golden file name ('unset-region.golden'). Please update the fixture name in the test or rename the golden file for consistency.
{args: "flink region unset", fixture: "flink/region/unset.golden", exitCode: 0}
sgagniere
reviewed
Jul 17, 2025
Comment on lines
+37
to
+41
| *.summary | ||
| /*report.xml | ||
| /test.summary | ||
| /*.html | ||
| /test/coverage/**/* |
Member
There was a problem hiding this comment.
We'll want to put this above line 15 because everything below that is managed by automation, which will probably revert this.
Member
There was a problem hiding this comment.
This file looks unused; can we remove it?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This has no breaking changes or customer facing features.
Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.What
gotestsumruns both in the Semaphore pipeline and locally..gitignoreto include other coverage artifacts.Blast Radius
The blast radius here is nil. At worst, it will cause unit or integ tests to fail or for the coverage to run improperly.
References
N/A
Test & Review
It is tests, so it has indeed been tested.