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: "fill attributes of swagger schema if provided for messages" #849

Merged
merged 7 commits into from
Jan 10, 2019

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Jan 9, 2019

Back-filling tests for #799:

  • a_bit_of_everything.proto: add 'example' example
  • add basic tests
  • test nested messages maybe next time 😅

@codecov-io
Copy link

codecov-io commented Jan 9, 2019

Codecov Report

Merging #849 into master will increase coverage by 1.42%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
+ Coverage   50.41%   51.83%   +1.42%     
==========================================
  Files          39       39              
  Lines        3765     3764       -1     
==========================================
+ Hits         1898     1951      +53     
+ Misses       1685     1629      -56     
- Partials      182      184       +2
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 48.08% <100%> (+5.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5ffc3b...a766ec3. Read the comment docs.

@johanbrandhorst
Copy link
Collaborator

Right then; re: test, I think it's good that we've got an example added to a_little_bit_of_everything.proto, but I'm wondering if we can add a test in https://github.com/grpc-ecosystem/grpc-gateway/blob/master/protoc-gen-swagger/genswagger/template_test.go that tests that populating the example field makes it into the output? What do you think?

@srenatus
Copy link
Contributor Author

srenatus commented Jan 9, 2019

Yup, will do

@srenatus
Copy link
Contributor Author

srenatus commented Jan 9, 2019

🔍 I couldn't find any obvious extension point -- it looks like only message fields have test scaffolding for the schemaCore bits, not messages. I'm happy to add a similar thing to message; is my basic understanding alright here?

@johanbrandhorst
Copy link
Collaborator

Seems about as good as my understanding of these tests 😬. Please try and add a new structure within which we can test the messages :). Also, maybe add some more examples in a_little_bit_of_everything.proto, to illustrate that it's verbatim JSON that is the format expected.

@birdayz
Copy link
Contributor

birdayz commented Jan 9, 2019

Thanks a lot for taking over! I didn't find the time to finish it. Also, my code was lacking one thing: setting the example value on field level, as the protobuf option doesn't make it possible (see my old comment in #799). Do you think you could work on that as well? Otherwise, you can only set the example for the WHOLE rpc, which is annoying i think.

tiny remark: would be great if you would keep my commits in the history as i wrote the initial patch.

@achew22
Copy link
Collaborator

achew22 commented Jan 9, 2019

@birdayz I'm in strong agreement that your contribution shouldn't be lost. I will make an exception to the single commit/PR rule to ensure that you get credit for your hard work. Thanks so much for contributing to the project. Without people like you this couldn't work

@srenatus
Copy link
Contributor Author

srenatus commented Jan 9, 2019 via email

@srenatus
Copy link
Contributor Author

srenatus commented Jan 9, 2019

Alternatively, we could merge #799 as-is and I'll put up a test-only PR following that.

@johanbrandhorst
Copy link
Collaborator

Yeah, let's merge the original and add tests asynchronously, that'd be the easiest solution.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
@johanbrandhorst
Copy link
Collaborator

This is great, thanks for taking the lead on this @srenatus. Looks like we've got a bazel failure. You should be able to fix it like so:

docker run -itv $(pwd):/grpc-gateway -w /grpc-gateway --entrypoint /bin/bash --rm \
    l.gcr.io/google/bazel -c 'bazel run :gazelle_fix; bazel run :buildifier'

@srenatus srenatus changed the title fill example attribute of swagger schema if provided for messages test: "fill attributes of swagger schema if provided for messages" Jan 10, 2019
Originally, I wanted this to become a test for nested messages; but I
didn't make it that far.

Anyhow, I think this way of testing this is better than what I had in
this PR before.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
@johanbrandhorst
Copy link
Collaborator

Great work on this, thanks! Increasing the coverage of that file by 5%!

@johanbrandhorst johanbrandhorst merged commit 20e8cf9 into grpc-ecosystem:master Jan 10, 2019
@srenatus srenatus deleted the swagger_example_field branch January 10, 2019 12:44
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.

6 participants