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

relabel: move tests to separate package #14077

Closed
wants to merge 3 commits into from

Conversation

tesla59
Copy link
Contributor

@tesla59 tesla59 commented May 10, 2024

Move relabel package tests to a separate package relabel_test. Also rename the tests to clearly indicate what they are testing and benchmarking

…to BenchmarkProcess

We should be naming the test functions based on the function they are testing.
TestRelabel was testing the Process function, so it should be named TestProcess.
Additionally, relabel is an unexported function so it should have its test function as Test_relabel instead.

Signed-off-by: tesla59 <nishant@heim.id>
Signed-off-by: tesla59 <nishant@heim.id>
Relabel is the name of package so can be removed from the variable name.

Signed-off-by: tesla59 <nishant@heim.id>
@tesla59
Copy link
Contributor Author

tesla59 commented May 10, 2024

please review @machine424

@beorn7
Copy link
Member

beorn7 commented May 15, 2024

The renaming seems to make sense.

But what's the motivation for moving the tests into a separate package? I know the pattern exists, and some advocate it because it forces tests to only use exported parts of the package to test. But it also adds some friction, so I think there should be some motivation beyond "just follow the pattern". AFAIK we don't regularly use the pattern in the Prometheus codebase. Cases that come to mind have to do with avoiding circular package dependencies.

@machine424 maybe you have discussed this with @tesla59 in advance?

@tesla59
Copy link
Contributor Author

tesla59 commented May 15, 2024

@beorn7 The motivation comes for this PR #13999
Because promql is following the _test I thought it can be extended to other part as well

@beorn7
Copy link
Member

beorn7 commented May 15, 2024

#13999 introduced the promql_test package to avoid a circular dependency between packages. We should not take it as a reason to now move all tests to their separate ..._test package.

I mean, maybe we want that, but then it should be an informed decision taken by the team. For now, I would assume the current state is the desired one and not change it.

@machine424
Copy link
Collaborator

Thank you for your initiatives and efforts, @tesla59.
I agree with @beorn7, I don’t think moving the tests into promql_test is necessary here.
Regarding the renaming, unless it’s really needed (the test was changed dramatically), I lean towards not changing the names to avoid losing the test’s history. If the test becomes flaky or something, we can look in the issues to check if we had that before, we can easily find threads related to it in Slack, etc. The same goes for benchmarks, we can still compare the results with the previous runs, etc.

Also, you can always start discussions on Stack or here to test the waters before opening a PR. We can also have discussions on the PRs themselves though, as you like. ;)

@beorn7
Copy link
Member

beorn7 commented May 16, 2024

@machine424 made a good point about not renaming tests unless there is a very compelling reason.

So let's close this PR. @tesla59, this is of course not meant to discourage you from further contributions.

@beorn7 beorn7 closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants