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

test: fix tests after requiring correct path #34

Merged
merged 47 commits into from
Feb 2, 2023
Merged

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Jan 26, 2023

Fix tests failures introduced in #32.

It's a huge PR. I'll summarize the work done:

  • Modify makefiles with correct cds so that the import paths matches the proto files' package names. (mostly done with sed magic + human cleanup after).
  • Sometimes, also modify the package name when they are not logical, example.
  • Modify some _test.go files which hardcoded old package names, example
  • Add some option go_package = "..."; so that the generated go package name matches other manually coded files in the same output folder.
  • Rename some folders with - to _ because proto package name doesn't allow -.

Allow changes above are "maintenance" updates. There are however 2 non-trivial changes:

Base automatically changed from am/correct-import-path to main January 30, 2023 16:58
@amaury1093 amaury1093 changed the title tests: fix tests after requiring correct path test: fix tests after requiring correct path Jan 31, 2023
Copy link
Contributor Author

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

I'm going to mark this PR as R4R.

All tests pass with protoc >=3.19.4, and the SDK uses 3.21.*.

It doesn't pass with older versions like protoc 3.0.2 and 2.6.1. I personally don't think it's worth pursuing more time fixing tests to make old versions happy, since all Cosmos chains should use recent versions of protoc. If that's fine with you, I need an admin to remove the "Required" check on github for the pb-3.0.2 and pb-2.6.1 job

Comment on lines +72 to +76
// Replace package name
re := regexp.MustCompile(`\npackage (.*);`)
correctPkgName := strings.Trim(strings.ReplaceAll(folder, "/", "."), ".")
replaceStr := fmt.Sprintf("\npackage $1.%s;", correctPkgName)
content = re.ReplaceAllString(content, replaceStr)
Copy link
Contributor Author

@amaury1093 amaury1093 Feb 1, 2023

Choose a reason for hiding this comment

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

Non-trivial hack/fix.

protoc-gen-combo generates, from a single proto file, 3 or 4 other derived proto files in subfolders, with minimal changes that are defined below in this file. It simply copies the original file, and modifies the pasted file using simple string search and replace.

I added this piece of logic here so that the derived proto file has correct package name.

expectedPrefix := strings.ReplaceAll(*f.Package, ".", "/") + "/"
if !strings.HasPrefix(*f.Name, expectedPrefix) {
panic(fmt.Errorf("file name %s does not start with expected %s; please make sure your folder structure matches the proto files fully-qualified names", *f.Name, expectedPrefix))
if f.Package != nil {
Copy link
Contributor Author

@amaury1093 amaury1093 Feb 1, 2023

Choose a reason for hiding this comment

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

With #32, empty package name are not allowed anymore. Probably that's a regression introduced in #32.

I added this if check on top of #32 to still allow empty package name, so that we don't stray away too much from gogo's original behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Can we have a new tag then after this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say let's tag after #37? Around next week. Or we can do 2 consecutive tags too.

Copy link
Member

Choose a reason for hiding this comment

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

Alright 👍🏾 I didn't see #37

@julienrbrt julienrbrt self-assigned this Feb 2, 2023
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

ACK!

Copy link

@testinginprod testinginprod left a comment

Choose a reason for hiding this comment

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

ack

@amaury1093 amaury1093 enabled auto-merge (squash) February 2, 2023 14:44
@amaury1093 amaury1093 merged commit 7f07934 into main Feb 2, 2023
@amaury1093 amaury1093 deleted the am/fix-tests-2 branch February 2, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants