Skip to content

[Test] Extract most PromQL test code into separate packages #13999

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
May 9, 2024
Merged

[Test] Extract most PromQL test code into separate packages #13999

merged 13 commits into from
May 9, 2024

Conversation

bboreham
Copy link
Member

It is undesirable to have the promql package contain a lot of test-only code, which depends on testify and other utilities.

I created three new packages:

  • promqltest, the test utilities used by promql tests and by other packages such as promtool.
  • promql_test which is most of the tests for promql package itself, in the same directory.
  • almost, which extracts almost.Equal used by test and non-test code.

I left some tests in the promql package, because they use a lot of unexported fields.
Possibly these indicate that more new packages should be extracted.

beorn7
beorn7 approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, epic work. And very nice. I think this makes a lot of sense.

I would assume this is uncontroversial, and I recommend to just merge. But I leave it to @bboreham to make a call if more feedback is required.

@beorn7 beorn7 mentioned this pull request May 2, 2024
Merged
@bboreham
Copy link
Member Author

bboreham commented May 3, 2024

I think the main people impacted by this are people using PromQL as a library outside of Prometheus.
And I don't know who they are, broadly. Maybe @charleskorn, @fpetkovski ?

Since I see I made a typo in one of the git commit headers, I will fix that and force-push.

@bboreham bboreham force-pushed the extract-promqltest branch from 5287992 to fc7fe82 Compare May 3, 2024 10:20
@fpetkovski
Copy link
Contributor

We run those tests as a library, but moving them to a separate package sounds like a good idea.

@pracucci
Copy link
Contributor

pracucci commented May 3, 2024

promql_test which is most of the tests for promql package itself, in the same directory.

I don't understand why promql_test is needed. Which problem do we solve with it?

@bboreham
Copy link
Member Author

bboreham commented May 3, 2024

Without it we have a circular dependency between promql and promql/promqltest.

@pracucci
Copy link
Contributor

pracucci commented May 3, 2024

Without it we have a circular dependency between promql and promql/promqltest.

I see. I don't know. From a design perspective, I'm not sure this change is a net positive benefit. To make it happen, you had to introduce promql_test which looks an anti-pattern to me, and expose some internal PromQL Engine features like the newly introduced TestXXX() functions which looks a bad smell.

I think the main people impacted by this are people using PromQL as a library outside of Prometheus.

To directly answer your question, looking at the changes I don't see any blocker for Mimir.

@bboreham
Copy link
Member Author

bboreham commented May 3, 2024

you had to introduce promql_test which looks an anti-pattern to me

Some people hold that this is actually a best practice.

It is described at https://pkg.go.dev/testing:

The test file can be in the same package as the one being tested, or in a corresponding package with the suffix "_test".

More discussion:

fpetkovski
fpetkovski reviewed May 3, 2024
View reviewed changes
promql/engine.go Outdated
@@ -584,6 +586,11 @@ func (ng *Engine) newTestQuery(f func(context.Context) error) Query {
return qry
}

func (ng *Engine) TestEnablePerStepStats() { ng.enablePerStepStats = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not possible to set these via engine opts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is possible, but there wasn't a way to get those options in to NewTestEngine.
I've pushed a commit which adds 20 false parameters so we can do it that way.
The other way would be for this one test to not call NewTestEngine.

@bboreham
Copy link
Member Author

bboreham commented May 4, 2024

Responding to Marco's point about exporting internals, I added parameters to NewTestEngine which allow TestEnablePerStepStats and TestSetLookbackDelta to be removed, exported the default to remove TestGetMaxSamplesPerQuery, and added hasAtModifier instead of exporting subqueryTimes.

Now the functions exported from engine.go in this PR are:

  • NewTestQuery - this is used to pass in functions which behave in special ways, e.g. blocking for a while. I don't see a straightforward way to achieve this in a black-box style.
  • TestSetMaxSamplesPerQuery - this I think could be replaced by re-creating the test engine.

@bboreham
Copy link
Member Author

bboreham commented May 4, 2024

Also one more export: KahanSum, a function only used by a test. Perhaps should be named TestKahanSum.
I think the feature can be tested instead via sum_over_time.

@charleskorn
Copy link
Contributor

I think the main people impacted by this are people using PromQL as a library outside of Prometheus. And I don't know who they are, broadly. Maybe @charleskorn, @fpetkovski ?

These changes look fine to me, I don't think they'll cause any issues for us.

@bboreham
Copy link
Member Author

bboreham commented May 8, 2024

I have now removed all new exports from promql in this PR, except NewTestQuery.
Also I renamed a new file engine_internal_test.go since I read that some people use this idiom.

If no further comments I will clean up the commits a little and merge.

bboreham added 11 commits May 8, 2024 13:42
@bboreham
refactor: extract almost.Equal() to new package
a1af3c2 
To avoid a circular reference between promql and promqltest.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: turn TestKahanSum into scripted test
2b0c87b 
This saves having a function solely to call kahanSumInc.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: check for @-modifier without using engine internals
a6e4276 
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: move test files into new promqltest package
11b27d5 
So that promql package does not bring in test-only dependencies.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: move promqltest tests together with the implementation
ccf7326 
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: make field initializers explicit
9aa321d 
Lint started complaining after I moved the file.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: clean up promqltest package references
b3c15d2 
So it nearly compiles.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
refactor: Move NewTestEngine into promqltest
babfcfd 
And export `DefaultMaxSamplesPerQuery` so callers can replicate previous
behaviour.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
promql: export NewTestQuery
e7c77f7 
So that tests can call it from another package.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: add promqltest package references
8fd9624 
To packages outside of promql.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
refactor: extract some PromQL Engine tests which use unexported structs
4a72607 
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham
test: move most PromQL tests into separate test package
0dbfd20 
So that they can import promqltest which imports promql.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham bboreham force-pushed the extract-promqltest branch from 2ffdf2f to ec4a6f1 Compare May 8, 2024 15:30
@bboreham
test: PromQL: stop using internal fields of engine
786e0e7 
* set enablePerStepStats and lookback duration via
  `NewTestEngine` parameters.
* check maxSamples by recreating query engine
* check lookback without modifying internals

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham bboreham force-pushed the extract-promqltest branch from ec4a6f1 to 786e0e7 Compare May 8, 2024 19:02
@bboreham bboreham merged commit 3fd24d1 into prometheus:main May 9, 2024
@bboreham bboreham deleted the extract-promqltest branch May 9, 2024 12:23
@charleskorn charleskorn mentioned this pull request May 10, 2024
Merged
@zenador zenador mentioned this pull request May 14, 2024
Merged
@tesla59 tesla59 mentioned this pull request May 15, 2024
Closed
@gotjosh gotjosh mentioned this pull request May 30, 2024
Merged
@charleskorn charleskorn mentioned this pull request Jul 1, 2024
Merged
@juliusv juliusv mentioned this pull request Jul 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beorn7 beorn7 beorn7 approved these changes

@dgl dgl Awaiting requested review from dgl dgl is a code owner

@roidelapluie roidelapluie Awaiting requested review from roidelapluie roidelapluie is a code owner

@cstyan cstyan Awaiting requested review from cstyan cstyan is a code owner

@bwplotka bwplotka Awaiting requested review from bwplotka bwplotka is a code owner

@tomwilkie tomwilkie Awaiting requested review from tomwilkie tomwilkie is a code owner

+1 more reviewer

@fpetkovski fpetkovski fpetkovski left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bboreham @fpetkovski @pracucci @charleskorn @beorn7