Port the orphaned Scala tests to TestNG and list all three licences in the POM by tfenne · Pull Request #1847 · samtools/htsjdk

tfenne · 2026-09-26T15:15:11Z

Port the five orphaned Scala test files to TestNG, and list all three licences in the published POM.

src/test/scala hasn't run since the Scala build was removed in 2022 (Removing scala test infrastructure #1640). Its cases are now TestNG tests in BamFileIoUtilsUnitTest, AbstractProgressLoggerTest, StringUtilTest, BasicFastqWriterTest and a new FastqReaderTest, and the directory is deleted. Two isBamFile expectations (test.Bam, test.BAM → false) were stale since extensions became case-insensitive and now assert true.
The POM listed only MIT; it now also lists Apache 2.0 (much of the CRAM code) and LGPL 2.1 (tribble, plus some FTP/seekable-stream classes) (Incomplete license metadata in build.gradle #1710).

Closes #1710.

Summary by CodeRabbit

Documentation
- Updated the 6.0.0 release notes to clarify that published package metadata lists Apache 2.0 and LGPL 2.1 alongside MIT.
Improvements
- Expanded automated checks for BAM filename recognition, FASTQ reading and writing, progress logging, and string utilities. These checks cover valid and invalid inputs, edge cases, and expected error behavior.

The five files under src/test/scala have not been compiled or run since the Scala build was removed (#1640). Every case is ported to a TestNG test in the test class of the class it tests: the isBamFile cases to BamFileIoUtilsUnitTest, the pad cases to a new AbstractProgressLoggerTest, the StringUtil cases to a new StringUtilTest, the FASTQ writer cases to BasicFastqWriterTest and the FASTQ reader cases to a new FastqReaderTest. Tables of cases that differ in meaning become separate tests, random bases come from a fixed-seed Random, and the tests use the Path APIs. isBamFile now accepts an extension in any case (#1808), so test.Bam and test.BAM are asserted to be BAM files where the Scala expected false.

The POM listed only MIT, but much of the CRAM code is under the Apache License 2.0 and the core Tribble code under the LGPL, as the README says. The Tribble headers name LGPL version 2.1. Fixes #1710.

coderabbitai · 2026-09-26T15:15:34Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: f5991611-76eb-40f7-a01c-ab3136cbfa5f

📥 Commits

Reviewing files that changed from the base of the PR and between 10c16c3 and f79f566.

📒 Files selected for processing (13)

CHANGELOG.md
build.gradle
src/test/java/htsjdk/TestClassDependenceTest.java
src/test/java/htsjdk/samtools/BamFileIoUtilsUnitTest.java
src/test/java/htsjdk/samtools/fastq/BasicFastqWriterTest.java
src/test/java/htsjdk/samtools/fastq/FastqReaderTest.java
src/test/java/htsjdk/samtools/util/AbstractProgressLoggerTest.java
src/test/java/htsjdk/samtools/util/StringUtilTest.java
src/test/scala/htsjdk/UnitSpec.scala
src/test/scala/htsjdk/samtools/BamFileIoUtilsTest.scala
src/test/scala/htsjdk/samtools/fastq/FastqReaderWriterTest.scala
src/test/scala/htsjdk/samtools/util/AbstractProgressLoggerTest.scala
src/test/scala/htsjdk/samtools/util/StringUtilTest.scala

💤 Files with no reviewable changes (5)

src/test/scala/htsjdk/samtools/util/StringUtilTest.scala
src/test/scala/htsjdk/UnitSpec.scala
src/test/scala/htsjdk/samtools/fastq/FastqReaderWriterTest.scala
src/test/scala/htsjdk/samtools/util/AbstractProgressLoggerTest.scala
src/test/scala/htsjdk/samtools/BamFileIoUtilsTest.scala

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📝 Walkthrough

Walkthrough

The publication POM adds Apache and LGPL license entries. Java test suites add coverage for BAM filename detection, FASTQ reading and writing, and utility methods. Corresponding Scala test suites and their shared base class are removed.

Changes

Publication license metadata

Layer / File(s)	Summary
Add publication license entries `build.gradle`, `CHANGELOG.md`	The published POM retains MIT and adds Apache License 2.0 and GNU Lesser General Public License 2.1. The changelog records the added entries.

Java test coverage

Layer / File(s)	Summary
Move BAM filename tests to Java `src/test/java/htsjdk/samtools/BamFileIoUtilsUnitTest.java`, `src/test/scala/htsjdk/samtools/BamFileIoUtilsTest.scala`	Java tests cover accepted and rejected `isBamFile` filename patterns. The Scala test suite is removed.
Move FASTQ tests to Java `src/test/java/htsjdk/samtools/fastq/BasicFastqWriterTest.java`, `src/test/java/htsjdk/samtools/fastq/FastqReaderTest.java`, `src/test/scala/htsjdk/samtools/fastq/FastqReaderWriterTest.scala`	Java tests cover writer output, reader round trips, malformed input, and reader iteration behavior. The Scala test suite is removed.
Move utility tests to Java `src/test/java/htsjdk/samtools/util/AbstractProgressLoggerTest.java`, `src/test/java/htsjdk/samtools/util/StringUtilTest.java`, `src/test/scala/htsjdk/samtools/util/AbstractProgressLoggerTest.scala`, `src/test/scala/htsjdk/samtools/util/StringUtilTest.scala`, `src/test/scala/htsjdk/UnitSpec.scala`, `src/test/java/htsjdk/TestClassDependenceTest.java`	Java tests cover `AbstractProgressLogger.pad` and `StringUtil` methods. The Scala suites and `UnitSpec` are removed, and a test comment no longer refers to `UnitSpec`.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to f79f5

The license metadata and test migration appear ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	Issue `#1710` covers published license metadata. The PR also ports five Scala test suites to Java, deletes `src/test/scala`, adds FASTQ and utility tests, and changes `isBamFile` expectations. These cha…	Move the test migration, Scala test deletion, and `isBamFile` expectation changes to a separate PR, or link coding issues that require those changes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 6 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes both primary changes: porting the orphaned Scala tests to TestNG and adding all three licenses to the published POM.
Linked Issues check	✅ Passed	Issue `#1710` requires additional published license metadata, including LGPL-2.1. `build.gradle` adds `Apache License, Version 2.0` and `GNU Lesser General Public License, Version 2.1` entries alongside…

Full details: Out of Scope Changes check

Explanation

Issue #1710 covers published license metadata. The PR also ports five Scala test suites to Java, deletes src/test/scala, adds FASTQ and utility tests, and changes isBamFile expectations. These changes have no demonstrated connection to the license metadata requirement. The changelog entry supports the license change and is in scope.

Full details: Docstring Coverage

Explanation

Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 6 files. (2 skipped: 2 unsupported.)

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

tfenne added 2 commits September 26, 2026 07:16

List the Apache 2.0 and LGPL 2.1 licences in the published POM

f79f566

The POM listed only MIT, but much of the CRAM code is under the Apache License 2.0 and the core Tribble code under the LGPL, as the README says. The Tribble headers name LGPL version 2.1. Fixes #1710.

Navigation Menu

Port the orphaned Scala tests to TestNG and list all three licences in the POM - #1847

Port the orphaned Scala tests to TestNG and list all three licences in the POM#1847
tfenne wants to merge 2 commits into
masterfrom
tf_build_test_hygiene

tfenne commented Sep 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 26, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

tfenne commented Sep 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tfenne commented Sep 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 26, 2026 •

edited

Loading