fix: Fixes migration tests for backup_online_archive #1724
Merged
Conversation
This file contains 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
AgustinBettati
approved these changes
Dec 7, 2023
internal/testutil/mig/provider.go
Outdated
| } | ||
| } | ||
|
|
||
| func ValueIfVersionAtLeast[T any](tb testing.TB, val T, minVersion string) T { |
There was a problem hiding this comment.
Suggested change
| func ValueIfVersionAtLeast[T any](tb testing.TB, val T, minVersion string) T { | |
| func ConfiguredProviderVersionAtLeast(tb testing.TB, minVersion string) bool |
nit: might find it easier to follow if this function returns a boolean, and then each function that generates a configuration uses that boolean to their preference (passing default value, or using the boolean itself to define a set of attributes in the config).
gssbzn
reviewed
Dec 7, 2023
internal/testutil/mig/pre_check.go
Outdated
Comment on lines
9
to
12
| func PreCheckBasic(tb testing.TB) { | ||
| checkLastVersion(tb) | ||
| acc.PreCheckBasic(tb) | ||
| } |
There was a problem hiding this comment.
you should consider enabling thelper in the linter, this and all test helpers should be
Suggested change
| func PreCheckBasic(tb testing.TB) { | |
| checkLastVersion(tb) | |
| acc.PreCheckBasic(tb) | |
| } | |
| func PreCheckBasic(tb testing.TB) { | |
| tb.Helper() | |
| checkLastVersion(tb) | |
| acc.PreCheckBasic(tb) | |
| } |
It will report the failing test correctly instead of flagging this function
There was a problem hiding this comment.
@gssbzn is there a problem if tb.Helper is called multiple times? e.g. one helper method calls another, but both can be called from tests.
There was a problem hiding this comment.
b.Helper is called multiple times?
No internally it flags to not consider the line but the caller the failure which will bubble up if a helper calls a helper
lantoli
commented
Dec 7, 2023
| @@ -98,6 +98,13 @@ linters: | |||
| - unconvert | |||
| - unused | |||
| - whitespace | |||
| - thelper | |||
There was a problem hiding this comment.
add some linters as in CLI
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fixes migration tests for backup_online_archive.
data_expiration_rulewas introduced in 1.12.2 so it can't be used before.It also refactors mig (migration) helper methods.
Link to any related issue(s):
Type of change:
Required Checklist:
Further comments