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

[3.3] Optimized testReadByte of FastJson2SerializationTest #15289

Open
wants to merge 1 commit into
base: 3.3
Choose a base branch
from

Conversation

zrlw
Copy link
Contributor

@zrlw zrlw commented Apr 1, 2025

What is the purpose of the change?

the milliseconds of variable date that used by testReadByte should not be zero, otherwise the output could be read as byte which will cause Assertions.assertThrows(IOException.class, objectInput::readByte) failure.

Error:  Tests run: 8, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 1.454 s <<< FAILURE! -- in org.apache.dubbo.common.serialize.fastjson2.FastJson2SerializationTest
Error:  org.apache.dubbo.common.serialize.fastjson2.FastJson2SerializationTest.testReadByte -- Time elapsed: 0.063 s <<< FAILURE!
org.opentest4j.AssertionFailedError: Expected java.io.IOException to be thrown, but nothing was thrown.
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
	at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:73)
	at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:35)
	at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3128)
	at org.apache.dubbo.common.serialize.fastjson2.FastJson2SerializationTest.testReadByte(FastJson2SerializationTest.java:236)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at java.util.ArrayList.forEach(ArrayList.java:1259)
	at java.util.ArrayList.forEach(ArrayList.java:1259)

Checklist

  • Make sure there is a GitHub_issue field for the change.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?

@zrlw
Copy link
Contributor Author

zrlw commented Apr 1, 2025

eg.

Date date = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").parse("2025-04-01 12:47:10");

it's output could be read as byte.

@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.77%. Comparing base (6a69ad4) to head (bb4373f).
Report is 5 commits behind head on 3.3.

Additional details and impacted files
@@            Coverage Diff            @@
##                3.3   #15289   +/-   ##
=========================================
  Coverage     60.77%   60.77%           
- Complexity    10896    10917   +21     
=========================================
  Files          1885     1886    +1     
  Lines         86087    86122   +35     
  Branches      12896    12902    +6     
=========================================
+ Hits          52317    52343   +26     
  Misses        28321    28321           
- Partials       5449     5458    +9     
Flag Coverage Δ
integration-tests 33.18% <ø> (+0.09%) ⬆️
samples-tests 29.23% <ø> (+0.02%) ⬆️
unit-tests 58.92% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Stellar1999
Copy link
Contributor

Stellar1999 commented Apr 1, 2025

I think this is not our test problem. Maybe it's a FastJson2 issue.
alibaba/fastjson2#3471

@zrlw
Copy link
Contributor Author

zrlw commented Apr 1, 2025

I think this is not our test problem. Maybe it's a FastJson2 issue. alibaba/fastjson2#3471

it seems that fastjson2 could take byte value from seconds or minutes timestamp, ObjectReaderImplByte#readJSONBObject will call JSONReaderJSONB#readInt32Value0 and return int32Value's byteValue.

            case BC_TIMESTAMP_MINUTES:
            case BC_TIMESTAMP_SECONDS:
            case BC_INT64_INT:
                int int32Value = getIntBE(bytes, check3(offset, end));
                offset += 4;
                return int32Value;

see details at https://github.com/alibaba/fastjson2/blob/main/core/src/main/java/com/alibaba/fastjson2/JSONReaderJSONB.java#L3625

@zrlw zrlw force-pushed the 3.3-optimized-FastJson2Test branch from d4425f6 to bb4373f Compare April 2, 2025 02:04
@zrlw
Copy link
Contributor Author

zrlw commented Apr 2, 2025

changed the date variable type at FastJson2SerializationTest#testReadByte from Date to LocalDate, because fastjson2 will support readMillis as int32, see details at alibaba/fastjson2@f5c0b62

@AlbumenJ
Copy link
Member

AlbumenJ commented Apr 7, 2025

changed the date variable type at FastJson2SerializationTest#testReadByte from Date to LocalDate, because fastjson2 will support readMillis as int32, see details at alibaba/fastjson2@f5c0b62

How about wait for Fastjson2 release?

@zrlw
Copy link
Contributor Author

zrlw commented Apr 8, 2025

changed the date variable type at FastJson2SerializationTest#testReadByte from Date to LocalDate, because fastjson2 will support readMillis as int32, see details at alibaba/fastjson2@f5c0b62

How about wait for Fastjson2 release?

FastJson2SerializationTest#testReadByte will always fail with the new version of Fastjson2 to be released because it will treat all Date types as byte type no matter the milliseconds part is zero or not.

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.

4 participants