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

smart diff to testutil.GatherAndCompare #998

Merged
merged 9 commits into from Apr 13, 2022

Conversation

sourikghosh
Copy link
Contributor

@sourikghosh sourikghosh commented Mar 7, 2022

Signed-off-by: Sourik Ghosh sourikghosh31@gmail.com
closes #996

Signed-off-by: Sourik Ghosh <sourikghosh31@gmail.com>
@sourikghosh
Copy link
Contributor Author

sourikghosh commented Mar 8, 2022

review @bwplotka

@bwplotka bwplotka requested review from bwplotka and kakkoyun Mar 16, 2022
Copy link
Member

@kakkoyun kakkoyun left a comment

Looking good.
I like to idea of using require.Equals but these are changing existing APIs. I'm reluctant to change existing public APIs.

How about adding equivalent new version of the APIs that have t *testing.T as a first parameter? and those can use require package under the hood.

sourikghosh added 2 commits Mar 17, 2022
Signed-off-by: Sourik Ghosh <sourikghosh31@gmail.com>
Signed-off-by: Sourik Ghosh <sourikghosh31@gmail.com>
@sourikghosh sourikghosh requested a review from kakkoyun Mar 17, 2022
Copy link
Member

@kakkoyun kakkoyun left a comment

Rather than MethodNameV2, I'd prefer MethodNameWithTesting or MethodNameWithT

@sourikghosh
Copy link
Contributor Author

sourikghosh commented Mar 17, 2022

Ok replacing it with MethodNameWithT
Ehh ci/circleci: go-1-16 for TestSummaryDecay failed but I run both make test and test-short locally before pushing.

Signed-off-by: Sourik Ghosh <sourikghosh31@gmail.com>
@sourikghosh sourikghosh requested a review from kakkoyun Mar 17, 2022
Copy link
Member

@bwplotka bwplotka left a comment

Hey, thanks for this work 💪🏽

It is MUST have functionality, I miss it so much <3

But I think we need to be careful. I think we could improve this PR a bit:

  • Ideally we don't force people to import extra (quite large) testify module. client_golang lib is well used library, so we need to be careful here. I think there is no strong need. Proposed alternative.
  • I am not fan of extra interface (so many extra methods). I think we can remove that and add nice diff without t *testing.T - especially if we want to skip testify module import.

WDYT?


"github.com/prometheus/common/expfmt"
"github.com/stretchr/testify/require"
Copy link
Member

@bwplotka bwplotka Mar 17, 2022

Choose a reason for hiding this comment

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

Hm.. is there a way to avoid this package? For a reason we wanted to make sure it's not used, so we are not leaking it.

Since this is not testing code, every project that imports client_golang needs to import testify.... I think we want to copy the diff function and maintain here.

See how we did that here https://github.com/efficientgo/tools/blob/main/core/pkg/testutil/testutil.go#L101

Copy link
Member

@kakkoyun kakkoyun Mar 17, 2022

Choose a reason for hiding this comment

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

I totally agree. I think I was a little hasty to review this, and I haven't noticed testify is not there already 🙈

Copy link
Member

@kakkoyun kakkoyun Mar 17, 2022

Choose a reason for hiding this comment

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

I guess because it's already a transitive dependency.

Copy link
Contributor Author

@sourikghosh sourikghosh Mar 18, 2022

Choose a reason for hiding this comment

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

yeah, I agree too, this makes sense
but the above diff function has a dep on
https://github.com/efficientgo/tools/blob/fe763185946be83b20da626605319733bd7f97cb/core/pkg/testutil/testutil.go#L16 which is not maintained should I transfer the module implementation over here or just simply import it.

Signed-off-by: Sourik Ghosh <sourikghosh31@gmail.com>
@sourikghosh sourikghosh requested review from bwplotka and kakkoyun Mar 19, 2022
Copy link
Member

@bwplotka bwplotka left a comment

Thanks! Sorry for complaining, but I think there is small risk with the module of choice 🤔 WDYT?

go.mod Outdated
github.com/golang/protobuf v1.5.2
github.com/json-iterator/go v1.1.12
github.com/pmezard/go-difflib v1.0.0
Copy link
Member

@bwplotka bwplotka Mar 28, 2022

Choose a reason for hiding this comment

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

I don't want to be a pain but this module is THIS PACKAGE IS NO LONGER MAINTAINED. Not the best, since all users depending on our module will have to download it too.

I wonder if it makes sense to just copy the code for now with the required code (with test) over. Looks reasonably small. There is not benefit in depending on module that does not allow merging new changes.

Or we could find different package.

Copy link
Contributor Author

@sourikghosh sourikghosh Mar 28, 2022

Choose a reason for hiding this comment

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

🙃that was my question previously #998 (comment)

Ideally, I would prefer an alternative to the package but I couldn't find a good replacement. So maybe coping small piece of code and maintaining it over here ?!!!

Copy link
Member

@bwplotka bwplotka Mar 29, 2022

Choose a reason for hiding this comment

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

Yea, I think copy might be better

@sourikghosh sourikghosh requested a review from bwplotka Mar 31, 2022
Signed-off-by: Sourik Ghosh <sourikghosh31@gmail.com>
@sourikghosh
Copy link
Contributor Author

sourikghosh commented Apr 1, 2022

@bwplotka 👀

@kakkoyun
Copy link
Member

kakkoyun commented Apr 13, 2022

@bwplotka Could you please have another look?

// It provides tools to compare sequences of strings and generate textual diffs.
//
// Maintaining `GetUnifiedDiffString` here because original repository
// is no loger maintained.(https://github.com/pmezard/go-difflib)
Copy link
Member

@bwplotka bwplotka Apr 13, 2022

Choose a reason for hiding this comment

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

Suggested change
// is no loger maintained.(https://github.com/pmezard/go-difflib)
// (https://github.com/pmezard/go-difflib) is no longer maintained.

Copy link
Member

@bwplotka bwplotka left a comment

Thanks for this work, and sorry for lag. Again - please ping me on CNCF slack (:

I found quite important thing to do before we merge, WDYT?

@@ -0,0 +1,649 @@
// Copyright 2022 The Prometheus Authors
Copy link
Member

@bwplotka bwplotka Apr 13, 2022

Choose a reason for hiding this comment

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

I believe we want to make sure this is under internal path so no one can import it. Otherwise we have to care about API, Go doc comments etc.

And I don't client_golang have to be library which offers diffing 🤔

Signed-off-by: Sourik Ghosh <sourikghosh31@gmail.com>
@sourikghosh sourikghosh requested a review from bwplotka Apr 13, 2022
Copy link
Member

@bwplotka bwplotka left a comment

Finally!

Thanks for your hard work 💪🏽

LGTM

@bwplotka bwplotka merged commit cd90f33 into prometheus:main Apr 13, 2022
8 checks passed
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.

3 participants