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

Allow empty response #383

Closed
wants to merge 20 commits into from

Conversation

stan-is-hate
Copy link
Contributor

@stan-is-hate stan-is-hate commented Feb 8, 2024

This PR allows empty message responses. One use case is grpc error responses, where you might want to ensure the server responded with no message but just the status.

TODOs:

  • make sure CI works (avro plugin didn't start correctly on my machine, but might be that it doesn't support arm macs)
  • do we want to allow empty responses only when metadata is not empty? and if so, how to do it? I think this should be handled in the rust part, not here. Here all we do is decode and pass the message into the test func.
  • why aren't matching rules supported in the metadata (might need a separate PR) Support metadata rules for sync message pact-reference#384
  • can this be tested via unit tests done, added unit tests
  • squash commits once reviewed (don't want to squash because changes might be required, wouldn't want to squash again)

@stan-is-hate stan-is-hate marked this pull request as ready for review February 10, 2024 01:01
@stan-is-hate stan-is-hate changed the title DRAFT: Allow empty response Allow empty response Feb 10, 2024
Copy link
Member

@mefellows mefellows left a comment

Choose a reason for hiding this comment

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

Couple of minor changes. Thanks so much for this 🙏


require.Error(t, err)
// TODO: uncomment once fixed on the ffi side
// require.ErrorContains(t, err, "no feature was found at latitude:-1 longitude:-1")
Copy link
Member

Choose a reason for hiding this comment

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

Just for clarity, are we dependent on pact-foundation/pact-reference#384 for this to be enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're dependent on this one instead: pact-foundation/pact-reference@29b326e
my fix is for rust tests, Ronald fixed it for FFI

func TestGrpcProvider(t *testing.T) {
go startProvider()
l.SetLogLevel("TRACE")
Copy link
Member

Choose a reason for hiding this comment

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

Probably we should bring this back, and adjust it so that all builds use the same log level. I can do this after bringing this in.

filepath.ToSlash(fmt.Sprintf("%s/PactGoV3Consumer-V3Provider.json", pactDir)),
filepath.ToSlash(fmt.Sprintf("%s/PactGoV2ConsumerMatch-V2ProviderMatch.json", pactDir)),
},
// BrokerURL: os.Getenv("PACT_BROKER_BASE_URL"),
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to leave these uncommented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think these changes crept in from various manual testing, I'll go through and clean them up tomorrow.

@coveralls
Copy link

coveralls commented Feb 13, 2024

Coverage Status

coverage: 37.243% (+0.04%) from 37.2%
when pulling 74c30a1 on stan-is-hate:allow-empty
into f04022c on pact-foundation:master.

stan-is-hate and others added 19 commits February 12, 2024 17:45
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 4 to 5.
- [Release notes](https://github.com/actions/setup-go/releases)
- [Commits](actions/setup-go@v4...v5)

---
updated-dependencies:
- dependency-name: actions/setup-go
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/spf13/cobra](https://github.com/spf13/cobra) from 1.7.0 to 1.8.0.
- [Release notes](https://github.com/spf13/cobra/releases)
- [Commits](spf13/cobra@v1.7.0...v1.8.0)

---
updated-dependencies:
- dependency-name: github.com/spf13/cobra
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/hashicorp/go-getter](https://github.com/hashicorp/go-getter) from 1.7.2 to 1.7.3.
- [Release notes](https://github.com/hashicorp/go-getter/releases)
- [Changelog](https://github.com/hashicorp/go-getter/blob/main/.goreleaser.yml)
- [Commits](hashicorp/go-getter@v1.7.2...v1.7.3)

---
updated-dependencies:
- dependency-name: github.com/hashicorp/go-getter
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.12.0 to 0.17.0.
- [Commits](golang/net@v0.12.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.58.1 to 1.60.1.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.58.1...v1.60.1)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/setup-java](https://github.com/actions/setup-java) from 3 to 4.
- [Release notes](https://github.com/actions/setup-java/releases)
- [Commits](actions/setup-java@v3...v4)

---
updated-dependencies:
- dependency-name: actions/setup-java
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github.com/spf13/afero](https://github.com/spf13/afero) from 1.9.5 to 1.11.0.
- [Release notes](https://github.com/spf13/afero/releases)
- [Commits](spf13/afero@v1.9.5...v1.11.0)

---
updated-dependencies:
- dependency-name: github.com/spf13/afero
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.11.0 to 0.17.0.
- [Commits](golang/crypto@v0.11.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@stan-is-hate
Copy link
Contributor Author

I'll close this one if you don't mind - I messed up the rebases and it's more hassle than it's worth to untangle them. I've opened #386 using a patch instead, so that one has a single commit :)

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.

5 participants