test: Implement fakes for prometheus api #1798
Conversation
Signed-off-by: Ying WANG <[email protected]>
Signed-off-by: Ying WANG <[email protected]>
Signed-off-by: Ying WANG <[email protected]>
ying-jeanne
requested review from
ArthurSens,
bwplotka,
kakkoyun and
vesari
as code owners
|
@ArthurSens @bwplotka @kakkoyun @vesari could you please give this PR a look? thank you. |
| "github.com/prometheus/common/model" | ||
| ) | ||
|
|
||
| func assertEqual(t *testing.T, a, b interface{}) { |
There was a problem hiding this comment.
cmp.Diff can be used (from github.com/google/go-cmp)
There was a problem hiding this comment.
This is a great start, thanks! I am not against this, but would love to explore alternatives before commit to this in our stable module.
- Should we decide what to do with API module first: proposal: Move API code to a separate module/repo #1649
- Should we auto-generate this instead of manual code.
- Should we explore closure/funcs approach. Currently with fake return params approach, tomorrow someone will complain that they want a specific result for a specific parameter or ensure some parameter invariance. Perhaps function based approach (See suggestion) might work better?
Also concerns from #1438 (comment) still apply.
| "github.com/prometheus/common/model" | ||
| ) | ||
|
|
||
| type FakeAPI struct { |
There was a problem hiding this comment.
If we follow this path we absolutely need to ensure this struct is not disconnected from API interface.
| type FakeAPI struct { | |
| var _ API = &FakeAPI{} | |
| type FakeAPI struct { |
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| package v1 |
There was a problem hiding this comment.
If we do this, let's show/test how it can be used for importers.
| package v1 | |
| package v1_test |
| fakeAPI := &FakeAPI{ | ||
| ExpectedLabelNamesResult: tt.expectedLabels, | ||
| ExpectedLabelNamesWarnings: tt.expectedWarnings, | ||
| ExpectedLabelNamesError: tt.expectedError, | ||
| } |
There was a problem hiding this comment.
Just to highlight what's currently possible. Instead of the referenced example one could write a following code
// Somewhere before the test
type myFakeAPI struct {
v1.API
labelNamesFn(ctx context.Context, matches []string, startTime, endTime time.Time, opts ...Option) ([]string, Warnings, error)
}
func (f *myFakeAPI) LabelNames(ctx context.Context, matches []string, startTime, endTime time.Time, opts ...Option) ([]string, Warnings, error) {
return f.labelNamesFn()
}
// No more methods needs to implemented, if the test uses only LabelNames....
// In your test...
fakeAPI := &myFakeAPI{
labelNamesFn: func(ctx context.Context, matches []string, startTime, endTime time.Time, opts ...Option) ([]string, Warnings, error) { return tt.expectedLabels,tt.expectedWarnings, tt.expectedError}
}Then the question is... is it worth to host fake impl if it's as simple as this?
| ExpectedLabelNamesResult []string | ||
| ExpectedLabelNamesWarnings Warnings | ||
| ExpectedLabelNamesError error |
There was a problem hiding this comment.
I wonder, wouldn't function based approach be much more flexible?
| ExpectedLabelNamesResult []string | |
| ExpectedLabelNamesWarnings Warnings | |
| ExpectedLabelNamesError error | |
| LabelNamesFn(ctx context.Context, matches []string, startTime, endTime time.Time, opts ...Option) ([]string, Warnings, error) |
Fixes: Prometheus GitHub issue #1438
This PR addresses Prometheus GitHub issue #1438 and the related comment by @bwplotka here. It introduces a simple fake implementation of the Prometheus API to facilitate unit testing. This fake API simulates the behavior of the actual Prometheus API, enabling unit tests without the need for a live Prometheus server. By combining this with end-to-end (e2e) tests, users will be able to cover all test cases comprehensively.
As an example of a similar approach, see this Grafana project.
For simplicity, the fake implementation is placed within the same package to avoid any import complexity for now.