cloud: Use static reference ID in tests #3061
Conversation
codebien
force-pushed
the
cloud/refactor/tests
branch
from
a6cf837
to
21f2c66
Compare
output/cloud/output_test.go
Outdated
| @@ -545,21 +488,12 @@ func TestCloudOutputRequireScriptName(t *testing.T) { | |||
|
|
|||
| func TestCloudOutputAggregationPeriodZeroNoBlock(t *testing.T) { | |||
There was a problem hiding this comment.
I am actually struggling to understand how this test was useful and what it checked even before your changes... 😅 It originated from #1130, but I think it got progressively mangled with subsequent refactorings 😕 With your latest changes, I am not sure it test anything meaningful beyond the fact that we can start and stop the output, which should already be covered by other tests 😕
There was a problem hiding this comment.
I've applied a basic fix by doing a simple push. I think the attempt here originally was to test the if AggregationPeriod is not set branch.
This test will not be used in v2 so I haven't applied any improvement. 8eb2506
There was a problem hiding this comment.
Yeah, but AggregationPeriod is not set by default, so presumably this is already covered by a bunch of tests, that's why I thought we can just get rid of it 😅 anyway, it will go away soon, so it doesn't really matter.
There was a problem hiding this comment.
I am joining the @na-- with missing the point of the original test and new additions that this PR is bringing to it.
There was a problem hiding this comment.
LGTM in general, though I think we can probably just move a lot of the current tests to v1/ as they are. A bunch of the tests are either not very high quality or are very specific to v1/ logic, so they can just be moved together with it and they can get discarded together with the v1/ logic.
Codecov Report
@@ Coverage Diff @@
## master #3061 +/- ##
==========================================
- Coverage 77.03% 76.73% -0.31%
==========================================
Files 229 229
Lines 17108 17110 +2
==========================================
- Hits 13180 13129 -51
- Misses 3085 3137 +52
- Partials 843 844 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I also applied the fixes for the linter, otherwise, we must fix them when we move the tests. It should be easier to review them here. |
|
I also removed |
Remove the dynamic resolution of the test' reference ID, and replaced it with a static injected value. It removes the dependency from all tests of the reference ID resolution. We will have a dedicated test for this logic making easier to control and have refactor around the cloud output.
codebien
force-pushed
the
cloud/refactor/tests
branch
from
9ec2e95
to
2cffc10
Compare
output/cloud/output_test.go
Outdated
| @@ -545,21 +488,12 @@ func TestCloudOutputRequireScriptName(t *testing.T) { | |||
|
|
|||
| func TestCloudOutputAggregationPeriodZeroNoBlock(t *testing.T) { | |||
There was a problem hiding this comment.
I am joining the @na-- with missing the point of the original test and new additions that this PR is bringing to it.
It is mostly a useless test: because the SUT should be already covered by other tests and we don't want to apply a refactor because it will be not used for the new cloud output.
Removed the lib.Options field from the cloud because it is not used.
codebien
force-pushed
the
cloud/refactor/tests
branch
from
2cffc10
to
c2efd34
Compare
The master's version mocks the test creation for all the tests doing them dependent on the resolution logic. The current PR removes this requirement and centralizes this assertion in a single test. Instead, all the other tests use a static-injected reference ID.
It should simplify the migration to something like the following architecture that I would do in #3041:
output/cloud/output.go: has the logic for the test creation and cleaning up (common logic)
Applying the current PR would be easier to eventually divide the unit tests of the test creation from the unit tests for metrics collection/aggregation/flushing logic across different components in the next PRs.