Skip to content

Conversation

@charleskorn
Copy link

The existing PromQL test cases test that label_replace and label_join return an error when the destination label name is invalid.

However, these test cases fail when UTF-8 name validation is enabled, and this is now the default as of prometheus/common#724.

In this PR, I've changed the invalid destination label name to something that is invalid with both the legacy and UTF-8 validation schemes. I've taken this value from the corresponding test cases in Prometheus itself.

…r legacy or UTF-8 name validation is enabled

Signed-off-by: Charles Korn <[email protected]>
@charleskorn charleskorn marked this pull request as ready for review September 15, 2025 01:18
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

because we still support both UTF-8 and legacy validation (and will do so forever, from my understanding), I create two types of tests:

  • failures under legacy but not UTF-8
  • failures under both legacy and UTF-8

So I would suggest making these tests in addition, rather than replacing, the existing legacy-failing tests

@charleskorn
Copy link
Author

because we still support both UTF-8 and legacy validation (and will do so forever, from my understanding), I create two types of tests:

  • failures under legacy but not UTF-8
  • failures under both legacy and UTF-8

So I would suggest making these tests in addition, rather than replacing, the existing legacy-failing tests

Thanks for the feedback. Do we need to exercise both groups in this test suite? My understanding is that this test suite isn't intended to exhaustively test every feature, but cover the important cases.

If you do want to cover both groups: is there an existing pattern for enabling / disabling particular test cases based on feature flags enabled on the target instance? I don't see one, but didn't want to make up something if I've missed something somewhere.

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.

2 participants