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

Fix cljam.io.bam-test to work properly #266

Merged
merged 4 commits into from Nov 28, 2022

Conversation

matsutomo81
Copy link
Contributor

@matsutomo81 matsutomo81 commented Nov 16, 2022

This PR resolves two issues included in the current cljam.io.bam-test.

First problem:

Current cljam.io.bam-test fails when run with JDK 19. This PR will fix cljam.io.bam-test to work regardless of JDK version.
Accordiing to this release note related to JDK 19, the result of Float.toString(float) evaluation may change from previous releases. The value of 1.17549435E-38, which is used in current test, is affected by this and It causes test failures. This PR resolves the problem by avoiding the use of 1.17549435E-38.

Second problem:

In the following code, there is a ?value at the end of the as-> macro, thus effectively evaluating (= ?value ?value). Perhaps a previous typo, which I have removed.

(are [?type ?value]
(= ?value
(as-> (#'encoder/encode-tag-value
(ByteBuffer/allocate 100) ?type ?value) ^ByteBuffer bb
(.position bb 0)
(decoder/parse-tag-single ?type bb) ?type ?value))

Same as above.
(are [?type ?value]
(= ?value
(as-> (#'encoder/encode-tag-value
(ByteBuffer/allocate 100) ?type ?value) ^ByteBuffer bb
(.position bb 0)
(decoder/parse-tag-single ?type bb) ?type ?value))

@matsutomo81 matsutomo81 requested review from alumi and a team as code owners November 16, 2022 11:21
@matsutomo81 matsutomo81 requested review from athos and removed request for a team November 16, 2022 11:21
@matsutomo81 matsutomo81 marked this pull request as draft November 16, 2022 11:29
@matsutomo81 matsutomo81 marked this pull request as ready for review November 17, 2022 02:50
@matsutomo81 matsutomo81 force-pushed the fix/bam_test_aux-data-codec-test branch from d477a60 to 731ccd5 Compare November 17, 2022 03:35
@matsutomo81
Copy link
Contributor Author

By referring to the log below, you can see that the current test fails on Java 19.
https://github.com/chrovis/cljam/actions/runs/3485121567/jobs/5830364082#step:7:305

Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

As I commented before, (= ?value ?value) doesn't check anything.
Fix the assertion first, then update the test cases. The linter warnings will be also resolved.

@alumi
Copy link
Member

alumi commented Nov 17, 2022

Uh, I see. (= ?value ?value) doesn't concern array types. Anyway, you can fix it in this PR since the goal of making the cljam.io.bam-test work properly is the same 👍

@@ -7,7 +7,7 @@ jobs:
runs-on: ubuntu-18.04
strategy:
matrix:
java: [ '8', '11', '16', '17-ea']
java: [ '8', '11', '16', '17-ea', '19']
Copy link
Member

Choose a reason for hiding this comment

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

LTS versions and the latest

Suggested change
java: [ '8', '11', '16', '17-ea', '19']
java: [ '8', '11', '17', '19']

@matsutomo81 matsutomo81 changed the title Change the value used in bam-test Fix cljam.io.bam-test to work properly Nov 17, 2022
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@alumi alumi requested review from a team and niyarin and removed request for athos and a team November 17, 2022 08:20
@alumi
Copy link
Member

alumi commented Nov 17, 2022

I just changed the setting of reviewers assignment from load balancing to round robin and redrew a reviewer.

Copy link
Contributor

@niyarin niyarin left a comment

Choose a reason for hiding this comment

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

LGTM.
Sorry for the late review.

@alumi alumi merged commit ab548fb into master Nov 28, 2022
@alumi alumi deleted the fix/bam_test_aux-data-codec-test branch November 28, 2022 04:48
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.

None yet

3 participants