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

gax-java(windows): HttpRequestRunnableTest.testUnicodeValuesInBody not passing for windows #1532

Closed
emmileaf opened this issue Mar 22, 2023 · 7 comments · Fixed by #1533
Closed
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: process A process-related concern. May include testing, release, or the like.

Comments

@emmileaf
Copy link
Contributor

This test looks like a more consistent source of failure for windows nightly builds, opening a single issue here to track and fix:

Error:  Failures: 
Error:    HttpRequestRunnableTest.testUnicodeValuesInBody:242 expected:
    number: 2
    name: "feline \342\230\272 \342\206\222 \342\206\220"
    type_url: "small \342\230\272 \342\206\222 \342\206\220"
    json_name: "mouse \342\230\272 \342\206\222 \342\206\220"
    default_value: "bird \342\230\272 \342\206\222 \342\206\220"
    
but was:
    number: 2
    name: "feline \303\242\313\234\302\272 \303\242\342\200\240\342\200\231 \303\242\342\200\240\357\277\275"
    type_url: "small \303\242\313\234\302\272 \303\242\342\200\240\357\277\275"
    json_name: "mouse \303\242\313\234\302\272 \303\242\342\200\240\342\200\231 \303\242\342\200\240\357\277\275"
    default_value: "bird \303\242\313\234\302\272 \303\242\342\200\240\342\200\231 \303\242\342\200\240\357\277\275"
    
[INFO] 
Error:  Tests run: 119, Failures: 1, Errors: 0, Skipped: 0

The com.google.api.gax.httpjson.HttpRequestRunnableTest.testUnicodeValuesInBody test fails on windows due to what looks like a octal sequence discrepancy:

https://github.com/googleapis/gapic-generator-java/blob/3922de4251924c5b765eba1319c253f13671a96f/gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpRequestRunnableTest.java#L205-L212

https://github.com/googleapis/gapic-generator-java/blob/3922de4251924c5b765eba1319c253f13671a96f/gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpRequestRunnableTest.java#L242

cc/ @lqiu96, if you have any context on this test from #1477?

@emmileaf emmileaf added type: process A process-related concern. May include testing, release, or the like. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Mar 22, 2023
@lqiu96
Copy link
Contributor

lqiu96 commented Mar 22, 2023

Thanks for digging @emmileaf! I was also just looking at this haha. For context, I was trying to resolve this issue: #1437

I think you're right about the octal sequence discrepancy on Windows. I'm not too familiar with this so I'm still trying to piece together why this didn't fail on linux/ why the escaped values passed locally.

I was playing around with this locally (on my Mac) and I think one option is to just manually convert this back to a String with charset UTF-8.
i.e.

      System.out.println(new String(result.toByteArray(), StandardCharsets.UTF_8));
      System.out.println(new String(bodyRequestMessage.toByteArray(), StandardCharsets.UTF_8));

Not ideal, but at least this would allow us to test that it does encode properly when creating the HttpRequest.

@lqiu96
Copy link
Contributor

lqiu96 commented Mar 22, 2023

Actually on a second look, perhaps the better fix is to ensure that the byteArrayOutputStream creates the String with UTF_8.

The Javadocs seems to suggest that the toString function uses the platform's default charset: https://docs.oracle.com/javase/8/docs/api/java/io/ByteArrayOutputStream.html#toString--

Windows seems to run with UTF-16, but generating the string with UTF-16 gives me an error. Running it with StandardCharsets.ISO_8859_1 gives me (which looks like the error in the nightly builds but is slightly different):

    number: 2
    name: "feline \303\242\302\230\302\272 \303\242\302\206\302\222 \303\242\302\206\302\220"
    type_url: "small \303\242\302\230\302\272 \303\242\302\206\302\222 \303\242\302\206\302\220"
    json_name: "mouse \303\242\302\230\302\272 \303\242\302\206\302\222 \303\242\302\206\302\220"
    default_value: "bird \303\242\302\230\302\272 \303\242\302\206\302\222 \303\242\302\206\302\220"

Perhaps something like might fix it?

      String output = byteArrayOutputStream.toString(StandardCharsets.UTF_8.toString());

@emmileaf
Copy link
Contributor Author

Perhaps something like might fix it?

      String output = byteArrayOutputStream.toString(StandardCharsets.UTF_8.toString());

Ah, that's probably it, thanks @lqiu96 for the investigation! I had trouble reproducing it with a mac setup as well, and was also looking for which step could be relying on an OS-specific default charset. Let's open a PR with this change and see if the error persists in the next nightly?

@lqiu96
Copy link
Contributor

lqiu96 commented Mar 22, 2023

Created a PR here: #1533 and added you as reviewer. We can trigger the nightly build after this going in and see if it works.

@lqiu96
Copy link
Contributor

lqiu96 commented Mar 22, 2023

@lqiu96
Copy link
Contributor

lqiu96 commented Mar 22, 2023

Thanks for the help @emmileaf! The nightly builds passed.

@emmileaf
Copy link
Contributor Author

Woot, thanks so much for the fix @lqiu96!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: process A process-related concern. May include testing, release, or the like.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants