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

Fixed JSPB test failures #2400

Conversation

acozzette
Copy link
Member

No description provided.

@acozzette
Copy link
Member Author

@xfxyjwf

@@ -1049,13 +1049,7 @@ describe('Message test suite', function() {
var nested = new proto.jspb.test.Deeply.Nested.Message();
nested.setCount(5);
msg.setDeeplyNestedMessage(nested);

// After a serialization-deserialization round trip we should get back the
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this serialize/deserialize roundtrip removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found that we don't currently generate fromObject methods (the code for it exists in js_generator.cc but is never called), so it seemed simplest to just avoid doing the round trip.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it still have the same test coverage? I thought the original bug can only be revealed after a round-trip. Is it not the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I went ahead and added the binary serialization round trip back in.

@acozzette acozzette merged commit 72002d8 into protocolbuffers:down-integrate-with-msvc-fix Nov 22, 2016
@acozzette acozzette deleted the jspb-test-fixes branch November 22, 2016 16:48
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.

3 participants