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

Ruby - Add failing tests for issue with deserializing wrapped values #7037

Closed

Conversation

djquan
Copy link
Contributor

@djquan djquan commented Dec 19, 2019

This is in reference to #7029.

The expected behavior is
1.) wrapped values with the default value set should serialize to a wrapped value with the default value set, but instead, they are being set to nil.

The first test fails with:
Error: test_map_wrappers_with_default_values(BasicTest::MessageContainerTest): NoMethodError: undefined method value for nil:NilClass, which matches what #7029 is seeing.

2.) The second test I added for completeness. A test that asserts the correct behavior for wrapped values with no value set.

The test structure followed the test introduced by @haberman here, #6797. My understanding of C is limited, but that PR seems to be one of the PRs that touched the Ruby code between the upgrade from 3.10.1 to 3.11.0, where this issue was introduced.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@djquan
Copy link
Contributor Author

djquan commented Dec 26, 2019

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@haberman
Copy link
Member

Thank you for these test cases. I incorporated them into my fix in #7195.

Could you give your consent to have your changes incorporated into that PR?

Some of the test cases weren't quite right though. Map values are always present, so m.map_double[0] will never return nil. I corrected that in my PR.

Thanks for submitting and sorry the fix was a bit slow.

@djquan
Copy link
Contributor Author

djquan commented Feb 11, 2020

@haberman Looks great, thanks! I consent to having my changes incorporated in #7195

Thanks again!

@haberman
Copy link
Member

@djquan for the system to recognize it, you need to put @googlebot I consent into a message on #7195.

See: #7195 (comment)

@djquan
Copy link
Contributor Author

djquan commented Feb 11, 2020

Ahh @haberman , gotcha, done!

@haberman
Copy link
Member

@djquan Thanks!

@acozzette
Copy link
Member

Closing this since the changes were incorporated into #7195.

@acozzette acozzette closed this Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants