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.

Sign up for GitHub

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

Jump to bottom

Add test cases for checkExternalDecimal() #152

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Add test cases for checkExternalDecimal() #152

merged 2 commits into from
Apr 15, 2024

Conversation

VermaSh
Copy link
Contributor

@VermaSh VermaSh commented Jan 10, 2024

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 11, 2024 •
edited
Loading

Launched a grinder to verify the change;

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 11, 2024

Launched another grinder because previous timed out.

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 17, 2024 •
edited
Loading

Launched a new grinder with just the DAA tests and longer timeout value

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 17, 2024

Grinder passed without any failures. @r30shah This is ready for review.

@VermaSh VermaSh mentioned this pull request Jan 17, 2024
Merged
@VermaSh VermaSh changed the title WIP: Add test cases for checkExternalDecimal() Add test cases for checkExternalDecimal() Jan 17, 2024
@VermaSh VermaSh force-pushed the checkExternalDecimalTests branch from c8864d4 to 7a9fc93 Compare January 17, 2024 13:49
@VermaSh VermaSh force-pushed the checkExternalDecimalTests branch from 9732331 to 0ad5885 Compare February 20, 2024 15:47
@VermaSh VermaSh force-pushed the checkExternalDecimalTests branch from 0ad5885 to 3677515 Compare February 22, 2024 16:21
r30shah
r30shah requested changes Apr 2, 2024
View reviewed changes
openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated
Assert.assertEquals("bytesWithSpaces < 0 should result in return code of 3", 3, result);

result = ExternalDecimal.checkExternalDecimal(a, 2, 5, DecimalData.EBCDIC_SIGN_EMBEDDED_TRAILING, 1);
Assert.assertEquals("bytesWithSpaces < 0 should result in return code of 3", 2, result);
Copy link
Contributor

Choose a reason for hiding this comment

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

We expect this test case to return 2 or 3? Comment and expected value does not match here.

openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated
ExternalDecimal.checkExternalDecimal(a, 0, -5, DecimalData.EBCDIC_SIGN_EMBEDDED_TRAILING, 5);
}
catch (IllegalArgumentException e) {
e.getMessage().contains("Precision must be greater than 0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming this is testing for IllegalArgumentException, shouldn't we pass or fail based on the result. Also contains method call do return boolean. Shouldn't we compare it ?

openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated
ExternalDecimal.checkExternalDecimal(a, 0, 0, DecimalData.EBCDIC_SIGN_EMBEDDED_TRAILING, 5);
}
catch (IllegalArgumentException e) {
e.getMessage().contains("Precision must be greater than 0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as previous.

openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated
ExternalDecimal.checkExternalDecimal(a, -1, 3, DecimalData.EBCDIC_SIGN_EMBEDDED_TRAILING, 5);
}
catch (IllegalArgumentException e) {
e.getMessage().contains("Offset must be non-negative integer");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as previous.

openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated
ExternalDecimal.checkExternalDecimal(a, 0, 3, 23, 5);
}
catch (IllegalArgumentException e) {
e.getMessage().contains("Invalid decimalType");
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as previous.

openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated
ExternalDecimal.checkExternalDecimal(a, 0, 9, DecimalData.EBCDIC_SIGN_EMBEDDED_TRAILING, 5);
}
catch (ArrayIndexOutOfBoundsException e) {
e.getMessage().contains("Array access index out of bounds");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as previous.

openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated
int result = ExternalDecimal.checkExternalDecimal(a, 0, 5, DecimalData.EBCDIC_SIGN_EMBEDDED_TRAILING, 1); // rc = 2
Assert.assertEquals("Result code for signEmbeddedTrailingPrefered(a) should have been 0", 0, result); // hardware returns 2

// Debugging - Start
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this comment?

openj9.test.daa/src/test.daa/net/openj9/test/arithmetics/TestValidityChecking.java Outdated

// Debugging - Start
result = ExternalDecimal.checkExternalDecimal(a, 0, 5, DecimalData.EBCDIC_SIGN_EMBEDDED_TRAILING, 2); // rc = 0
// it honors DSC < DC but maybe it needs it to be +1 than actual value?
Copy link
Contributor

Choose a reason for hiding this comment

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

Also why do we have this comment ?

@VermaSh VermaSh force-pushed the checkExternalDecimalTests branch from 7a539af to 1585adb Compare April 3, 2024 19:18
@VermaSh
Changes based on PR suggestions
45bd5f5 
Signed-off-by: Shubham Verma <[email protected]>
@VermaSh VermaSh force-pushed the checkExternalDecimalTests branch from 1585adb to 45bd5f5 Compare April 3, 2024 19:38
@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 3, 2024

@r30shah I have updated the PR based on your suggestions. I have tested these changes locally with my openj9 change and no failures.

r30shah
r30shah approved these changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM, @VermaSh can you confirm if you have tested these changes with corresponding API changes in OpenJ9 (Which is merged now)?

@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 12, 2024

I have tested it with the openj9 PR but not with merged eclipse branch. Let me test it again with eclipse branches.

@r30shah
Copy link
Contributor

r30shah commented Apr 12, 2024

I have tested it with the openj9 PR but not with merged eclipse branch. Let me test it again with eclipse branches.
Ok, will merge once you confirm

@r30shah
Copy link
Contributor

r30shah commented Apr 15, 2024 •
edited
Loading

@VermaSh is this safe to merge? i.e, have you verified as mentioned in #152 (comment)?

@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 15, 2024

@r30shah yup, this is safe to merge.

@r30shah r30shah self-assigned this Apr 15, 2024
@r30shah
Copy link
Contributor

r30shah commented Apr 15, 2024

Thanks @VermaSh , merging changes based on the testing done so far.

@r30shah r30shah merged commit e8ca471 into adoptium:master Apr 15, 2024
2 checks passed
@llxia
Copy link
Contributor

llxia commented Apr 15, 2024

@r30shah do we expect this PR to be included in April release (0.44)? If so, we will need to re-tag this repo and update testenv.property file https://github.com/adoptium/aqa-tests/blob/v1.0.1-release/testenv/testenv.properties#L8

@r30shah
Copy link
Contributor

r30shah commented Apr 15, 2024

Hi @llxia , No, this PR adds unit test for the changes @VermaSh has made in the DAA api (eclipse-openj9/openj9#18272) which was merged after 0.44 and we do not plan to backport new API changes in 0.44.

Do we need to make any changes in that case?

@llxia
Copy link
Contributor

llxia commented Apr 15, 2024

No, if we do not plan to backport new API changes in 0.44, then no action is needed. Thanks @r30shah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r30shah r30shah r30shah approved these changes

Assignees

@r30shah r30shah

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@VermaSh @r30shah @llxia