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

Reorganize function tests #144

Open
GiedriusS opened this issue Jan 6, 2023 · 6 comments · May be fixed by #376
Open

Reorganize function tests #144

GiedriusS opened this issue Jan 6, 2023 · 6 comments · May be fixed by #376
Assignees
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@GiedriusS
Copy link
Member

GiedriusS commented Jan 6, 2023

#142 (review)

With lots of functions, it's hard to understand what is being tested and what is not. Reorganize the tests to make them clearer.

@GiedriusS GiedriusS added help wanted Extra attention is needed good first issue Good for newcomers labels Jan 6, 2023
@fatsheep9146
Copy link
Contributor

I'd love to help with this, what way of reorganizing do you suggest? @GiedriusS @fpetkovski

@fpetkovski
Copy link
Collaborator

First thing I would suggest is merging instant and range query tests, so that we add test cases in only one place. I would look into how we can define multiple expressions in a test case for a single load. This way we don't need to repeat the loaded series over and over when we add a new function.

@GiedriusS
Copy link
Member Author

I was thinking maybe we could cover more cases in the fuzzer https://github.com/thanos-community/promql-engine/blob/main/engine/enginefuzz_test.go#L21 and we could remove simple cases from that testing function #142 (review).

@rakeshkumarnahak
Copy link

/assign

@MichaHoffmann
Copy link
Contributor

I wonder, why does the enginefuzz_test.go not generate NaN or Inf values that seems like it could produce interesting edge cases!

@fpetkovski
Copy link
Collaborator

Up until recently we had no support for NaN comparisons since there are many different ways to represent a NaN in go. We could it it now though, and use cmpopts.EquateNaNs() as in https://github.com/thanos-community/promql-engine/blob/main/engine/engine_test.go#L1565.

@kartikaysaxena kartikaysaxena linked a pull request Jan 2, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants