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

Issue #12367 testCaseTransformFunctionWithIntResults test fix #12922

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

aditya0811
Copy link
Contributor

flaky test fix Issue #12367

@aditya0811
Copy link
Contributor Author

build was failing during checkstyle violation, fixed it

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.17%. Comparing base (59551e4) to head (e5ad786).
Report is 375 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #12922      +/-   ##
============================================
+ Coverage     61.75%   62.17%   +0.42%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2502      +66     
  Lines        133233   136670    +3437     
  Branches      20636    21164     +528     
============================================
+ Hits          82274    84977    +2703     
- Misses        44911    45410     +499     
- Partials       6048     6283     +235     
Flag Coverage Δ
custom-integration1 <0.01% <ø> (-0.01%) ⬇️
integration <0.01% <ø> (-0.01%) ⬇️
integration1 <0.01% <ø> (-0.01%) ⬇️
integration2 0.00% <ø> (ø)
java-11 62.13% <ø> (+0.42%) ⬆️
java-21 62.06% <ø> (+0.43%) ⬆️
skip-bytebuffers-false 62.15% <ø> (+0.41%) ⬆️
skip-bytebuffers-true 62.02% <ø> (+34.29%) ⬆️
temurin 62.17% <ø> (+0.42%) ⬆️
unittests 62.17% <ø> (+0.42%) ⬆️
unittests1 46.74% <ø> (-0.15%) ⬇️
unittests2 27.93% <ø> (+0.20%) ⬆️

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.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Can you also add a special test which will fail if the cast doesn't exist?

@aditya0811
Copy link
Contributor Author

Thanks for the fix! Can you also add a special test which will fail if the cast doesn't exist?

sure

@aditya0811
Copy link
Contributor Author

Thanks for the fix! Can you also add a special test which will fail if the cast doesn't exist?

Created a separate projectionBlock object to hold such float value, so as to not interfere with variables in the parent class.

@aditya0811 aditya0811 force-pushed the master branch 2 times, most recently from 8edbe38 to 254ce45 Compare April 22, 2024 16:51
@aditya0811
Copy link
Contributor Author

Hello @Jackie-Jiang, please let me know your thoughts on the newly added negative scenario test.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

There should be an easier way to test this behavior without creating a new segment:
We can loop over the random float values from _floatSVValues and pick a value that is floatValue != (float) Double.parseDouble(Float.toString(floatValue))

@aditya0811
Copy link
Contributor Author

There should be an easier way to test this behavior without creating a new segment: We can loop over the random float values from _floatSVValues and pick a value that is floatValue != (float) Double.parseDouble(Float.toString(floatValue))

Shouldn't something like 0.1f satisfy this condition?

@Jackie-Jiang
Copy link
Contributor

(double) 0.1f != 0.1, but 0.1f = (float) 0.1. I don't remember which way we do the conversion. Can you try it out and see if it breaks with 0.1?

@aditya0811
Copy link
Contributor Author

The new test case in PR has 0.1f, which breaks the existing test, however doesn't satisfy floatValue != (float) Double.parseDouble(Float.toString(floatValue)).

We compare float(_leftStorageType) with double(_rightStorageType). For _leftStorageType, with implicit cast, the float value=0.1f is converted to (double) value = 0.10000000149011612. For _rightStorageType, we parse it as double value= 0.1d.

@aditya0811
Copy link
Contributor Author

We can use following which breaks for 0.1f and pass by using CAST.

Double.compare(_floatSVValues[i], Double.parseDouble(String.format("%f", _floatSVValues[i])))!=0

@Jackie-Jiang Jackie-Jiang merged commit 7b06b9f into apache:master Apr 29, 2024
20 checks passed
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.

None yet

3 participants