Add script for converting PromQL tests to new syntax format #16562
Conversation
b386ab6 to
3171bb2
Compare
|
Tagging @beorn7 and @NeerajGartia21 |
There was a problem hiding this comment.
Thank you very much.
I still have to look at the code in detail. (Maybe @NeerajGartia21 will beat me to that. :)
However, one note: I don't think we should put this as a 1st class top level binary into the directory structure. This is just for a one time transition. I would put the file with the Go main package somewhere underneath the promqltest directory and give instructions in the README.md how to go run ... it.
About the actual conversion in this PR: This should be done in a separate commit from the conversion script. Maybe even in a separate PR. And we should do a manual pass through it to see where the "strict" or the "basic" conversion makes more sense. (Bonus points for separate commits: First the auto-conversion, presumably in strict mode, and then a separate commit to manually relax the checks where it makes sense.)
There was a problem hiding this comment.
As a follow-up to my previous comment, here one example each for both cases:
- We want to convert a simple
evalinto a new-style test withexpect no_info(orexpect no_warnin other cases) in cases where the absence of an annotation is relevant. - We want to convert an old-style
eval_infointo a simple new-styleevalin cases where we do not really care about annotations.
All the tests have to be judged similarly, as suggested in my previous comment. (It doesn't have to be rocket science, but it should generally make sense.)
|
I am right now hardcoding the directory path: "promql/promqltest/testdata", do we need to make it configurable? We have 3 modes now: tolerant, basic, strict and the indentation per eval block is consistent (using reference from existing indentation) so that the things don't look weird The main method is also now under promqltest folder with instructions in the corresponding README.md |
MigrateTestData is now part of new file- test_migrate.go with a doc comment on top Do we need 2 separate PR one using strict mode and one with the other mode? |
Signed-off-by: Kapil Lamba <kapillamba4@gmail.com>
Signed-off-by: Kapil Lamba <kapillamba4@gmail.com>
|
Let's first finish the review here and then think about the conversion PR. (I guess we should have one PR with two commits. First the auto-conversion and then the manual adjustments.) |
|
I'll try to look at the code here in detail ASAP. |
Indeed, the directory should be customizable by the user. This is mainly meant for converting tests outside of this repo. (For a one-time conversion just within this repo, we did not need to publish the script here as part of the repo.) |
There was a problem hiding this comment.
Overall, the code looks good. Apologies for the delayed review.
Additionally, it would be helpful to include tests for this script. This will make it easier for reviewers to verify the functionality without running it locally and will also benefit users by providing clear examples for how to use and test the script.
Signed-off-by: Kapil Lamba <kapillamba4@gmail.com>
Signed-off-by: Kapil Lamba <kapillamba4@gmail.com>
There was a problem hiding this comment.
Looks good to me! Just a small nit.
Thanks for the script @kapillamba4 !
Let's now wait for @beorn7's review.
Co-authored-by: Neeraj Gartia <80708727+NeerajGartia21@users.noreply.github.com> Signed-off-by: Kapil Lamba <kapillamba4@gmail.com>
|
Thanks everyone. And apologies for the review delay. I'm still caught between several events at work and in my usual life, but I will finally be back at reviewing code next week. |
Signed-off-by: Kapil Lamba <kapillamba4@gmail.com>
|
Thanks for reviewing @beorn7, I’ve implemented the doc updates you suggested. Have also rebased the strict migration mode PR with this PR |
Fix for #16367
Adding a simple script to convert tests automatically to new syntax.