Fix up unit tests so that gradle correctly reports results (no more erroneous skips) (#1759)

* Remove testng.xml that caused Gradle to report all tests as skipped.

The testng.xml in src/test/resources was auto-discovered by TestNG on the
classpath, causing it to run tests through its own parallel infrastructure
(parallel="methods" thread-count="8") instead of through Gradle's TestListener.
As a result, Gradle never received test completion events and reported all
~19,000 tests as "skipped" with 0 successes, even though they actually ran.

The build.gradle already configures TestNG properly via useTestNG {} blocks
with the correct group inclusions/exclusions, making this file redundant.

* Replace @BeforeTest/@AfterTest with @BeforeMethod/@AfterMethod in most tests.

Without testng.xml, TestNG puts all test classes into a single <test> scope.
A @BeforeTest failure in any class (e.g. HtsgetBAMFileReaderTest trying to
connect to a local htsget server) can cascade to skip every test in the entire
suite — resulting in 19,000+ tests reported as skipped with 0 successes.

* Auto-detect samtools on PATH when HTSJDK_SAMTOOLS_BIN is not set.

SamtoolsTestUtils.getSamtoolsBin() now checks: (1) the HTSJDK_SAMTOOLS_BIN
environment variable, (2) "which samtools" on the system PATH, (3) the
default /usr/local/bin/samtools. This allows tests that depend on samtools
(CRAM31Tests, HtsCRAMCodec31Test, SamtoolsTestUtilsTest) to pass without
requiring the environment variable when samtools is already installed.

* Add "htsget" test group and exclude it from the default test run.

Tests that require a local htsget server (HtsgetBAMFileReaderTest,
HtsgetBAMCodecTest) are now tagged with groups="htsget" and excluded
by default, alongside ftp/http/sra/ena. HTSGET permutations are also
removed from SamReaderFactoryTest's data provider since the dedicated
htsget test classes provide full coverage.

TestNGUtils.getDataProviders() now accepts excluded groups so that
TestDataProviders doesn't try to invoke data providers from classes
that require unavailable external infrastructure.

* Fix thread-safety issues in tests for parallel class execution.

JimFS: Replace hardcoded Jimfs.newFileSystem("test", ...) with
Jimfs.newFileSystem(Configuration.unix()) in 7 test files so concurrent
test classes don't collide on the filesystem name "jimfs://test".

BlockCompressedInputStreamTest: Pass CountingInflaterFactory directly to
each stream constructor instead of setting it as a global default via
BlockGunzipper.setDefaultInflaterFactory(). The global approach caused
flaky failures when other classes concurrently used the same global
InflaterFactory. The test now verifies the same behavior (custom inflater
is used and call count is correct) without touching shared state.

* Switch to TestNG parallel="classes" with 2x CPU threads.

Use TestNG's in-process parallel="classes" instead of Gradle's
maxParallelForks. Unlike parallel="methods" (which breaks Gradle's test
listener), parallel="classes" preserves correct event ordering since
onBeforeClass completes before any onTestStart for that class.

* Fix testExternalApis CI job and remove duplicate testFTP job.

testExternalApis was running ./gradlew testFTP (copy-paste error) instead
of ./gradlew testExternalApis (which runs the sra, http, and ena groups).
The testFTP job was an exact duplicate of the broken testExternalApis job,
both running the same testFTP gradle task. Removed it since the target FTP
site no longer exists.

Author: tfenne

.github/workflows/tests.yml

@@ -60,41 +60,13 @@ jobs:
       - name: Compile with Gradle
         run: ./gradlew compileJava
       - name: Run tests
-        run: ./gradlew testFTP jacocoTestReport
-#      - name: Upload to codecov
-#        run: bash <(curl -s https://raw.githubusercontent.com/broadinstitute/codecov-bash-uploader/main/codecov-verified.bash)
+        run: ./gradlew testExternalApis jacocoTestReport
       - name: Upload test results
         if: always()
         uses: actions/upload-artifact@v4
         with:
           name: test-results-external-apis
           path: build/reports/tests
-  testFTP:
-    continue-on-error: true
-    runs-on: ubuntu-latest
-    name: FTP tests
-    steps:
-      - uses: actions/checkout@v3
-      - name: Set up java 17
-        uses: actions/setup-java@v3
-        with:
-          java-version: '17'
-          distribution: 'adopt'
-          cache: gradle
-      - name: Grant execute permission for gradlew
-        run: chmod +x gradlew
-      - name: Compile with Gradle
-        run: ./gradlew compileJava
-      - name: Run tests
-        run: ./gradlew testFTP jacocoTestReport
-#      - name: Upload to codecov
-#        run: bash <(curl -s https://raw.githubusercontent.com/broadinstitute/codecov-bash-uploader/main/codecov-verified.bash)
-      - name: Upload test results
-        if: always()
-        uses: actions/upload-artifact@v4
-        with:
-          name: test-results-ftp
-          path: build/reports/tests
   spotBugs:
     runs-on: ubuntu-latest
     name: SpotBugs

build.gradle

@@ -84,13 +84,11 @@ import org.gradle.internal.os.OperatingSystem;
 tasks.withType(Test).configureEach { task ->
     task.outputs.upToDateWhen { false } // tests will always rerun
 
-    // Always run serially because there are some very badly behaved tests in HTSJDK that
-    // will cause errors and even deadlocks if run multi-threaded
     task.maxParallelForks = 1
 
     // set heap size for the test JVM(s)
     task.minHeapSize = "1G"
-    task.maxHeapSize = "2G"
+    task.maxHeapSize = "6G"
 
     task.jvmArgs '-Djava.awt.headless=true'  //this prevents awt from displaying a java icon while the tests are running
 
@@ -141,10 +139,12 @@ test {
 
    useTestNG {
        if (OperatingSystem.current().isUnix()) {
-           excludeGroups "slow", "broken", "defaultReference", "optimistic_vcf_4_4", "ftp", "http", "sra", "ena"
+           excludeGroups "slow", "broken", "defaultReference", "optimistic_vcf_4_4", "ftp", "http", "sra", "ena", "htsget"
        } else {
-           excludeGroups "slow", "broken", "defaultReference", "optimistic_vcf_4_4", "ftp", "http", "sra", "ena", "unix"
+           excludeGroups "slow", "broken", "defaultReference", "optimistic_vcf_4_4", "ftp", "http", "sra", "ena", "htsget", "unix"
        }
+       parallel = "classes"
+       threadCount = 2 * Runtime.runtime.availableProcessors()
    }
 } dependsOn testWithDefaultReference, testWithOptimisticVCF4_4
 

src/test/java/htsjdk/TestDataProviders.java

@@ -35,10 +35,13 @@ public void independentTestOfDataProviderTest() throws Exception {
                 "getDataProviders didn't find testAllDataProvidersData, which is in this class. Something is wrong.");
     }
 
+    /** Groups that require external infrastructure and are excluded from the default test run. */
+    private static final Set<String> EXCLUDED_GROUPS = Set.of("htsget", "ftp", "http", "sra", "ena");
+
     @DataProvider(name = "DataprovidersThatDontTestThemselves")
     private Iterator<Object[]> testAllDataProvidersData() throws Exception {
 
-        return TestNGUtils.getDataProviders("htsjdk");
+        return TestNGUtils.getDataProviders("htsjdk", EXCLUDED_GROUPS);
     }
 
     // runs all the @DataProviders it gets from DataprovidersThatDontTestThemselves.

src/test/java/htsjdk/beta/codecs/reads/htsget/HtsgetBAM/HtsgetBAMCodecTest.java

@@ -27,7 +27,7 @@
 import java.util.Iterator;
 
 // NOTE: running these tests require a local htsget server
-
+@Test(groups = "htsget")
 public class HtsgetBAMCodecTest extends HtsjdkTest {
     private final static IOPath htsgetBAM = new HtsPath(
             HtsgetBAMFileReaderTest.HTSGET_ENDPOINT +

src/test/java/htsjdk/beta/codecs/variants/vcf/HtsVCFCodecTest.java

@@ -69,7 +69,7 @@ private Object[][] vcfReadWriteTests() {
 
     @Test
     public void testRoundTripNio() throws IOException {
-        try(FileSystem fs = Jimfs.newFileSystem("test", Configuration.unix())){
+        try(FileSystem fs = Jimfs.newFileSystem(Configuration.unix())){
             final IOPath outputPath = IOUtils.createTempPath("roundTripVCFThroughPath", ".vcf");
             Path tribbleFileInJimfs = TestUtils.getTribbleFileInJimfs(TEST_VCF_WITH_INDEX.getRawInputString(), TEST_VCF_INDEX.getRawInputString(), fs);
             HtsPath vcf = new HtsPath(tribbleFileInJimfs.toUri().toString());

src/test/java/htsjdk/samtools/BAMFileReaderTest.java

@@ -4,7 +4,7 @@
 import htsjdk.samtools.util.CloseableIterator;
 import htsjdk.samtools.util.CoordMath;
 import org.testng.Assert;
-import org.testng.annotations.BeforeTest;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import java.io.File;
@@ -28,7 +28,7 @@ public class BAMFileReaderTest extends HtsjdkTest {
     private static BAMFileReader bamFileReaderWrong;
     private static BAMFileReader bamFileReaderNull;
 
-    @BeforeTest
+    @BeforeMethod
     public void init() throws IOException {
         bamFileReaderBAI = new BAMFileReader(bamFile, baiFileIndex, true, false, ValidationStringency.DEFAULT_STRINGENCY, DefaultSAMRecordFactory.getInstance());
         bamFileReaderCSI = new BAMFileReader(bamFile, csiFileIndex, true, false, ValidationStringency.DEFAULT_STRINGENCY, DefaultSAMRecordFactory.getInstance());

src/test/java/htsjdk/samtools/CRAMFileCRAIIndexTest.java

@@ -15,7 +15,7 @@
 import htsjdk.samtools.util.CoordMath;
 import htsjdk.samtools.util.IOUtil;
 import org.testng.Assert;
-import org.testng.annotations.BeforeTest;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import java.io.*;
@@ -322,7 +322,7 @@ public void testQueryIntervalWithFilePointers() throws IOException {
         }
     }
 
-    @BeforeTest
+    @BeforeMethod
     public void prepare() throws IOException {
         tmpCramFile = File.createTempFile(BAM_FILE.getName(), ".cram") ;
         tmpCramFile.deleteOnExit();

src/test/java/htsjdk/samtools/CRAMFileWriterWithIndexTest.java

@@ -9,7 +9,7 @@
 import htsjdk.samtools.util.CloseableIterator;
 import htsjdk.samtools.util.TestUtil;
 import org.testng.Assert;
-import org.testng.annotations.BeforeTest;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import java.io.ByteArrayOutputStream;
@@ -150,7 +150,7 @@ public void testUnnecessaryIO() throws IOException {
         Assert.assertTrue(iterator.hasNext());
     }
 
-    @BeforeTest
+    @BeforeMethod
     public void beforeTest() {
         header = new SAMFileHeader();
         header.setSortOrder(SAMFileHeader.SortOrder.coordinate);

src/test/java/htsjdk/samtools/CSIIndexTest.java

@@ -2,7 +2,7 @@
 
 import htsjdk.HtsjdkTest;
 import org.testng.Assert;
-import org.testng.annotations.BeforeTest;
+import org.testng.annotations.BeforeClass;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
@@ -39,7 +39,7 @@ public class CSIIndexTest extends HtsjdkTest {
     public static final int BAI_EQUIVALENT_MIN_SHIFT = 14;
     public static final int BAI_EQUIVALENT_BIN_DEPTH = 6;
 
-    @BeforeTest
+    @BeforeClass
     public void init() {
         bai = new DiskBasedBAMFileIndex(new File(TEST_DATA_DIR, "index_test.bam.bai"), null);
         csi = new CSIIndex(new File(TEST_DATA_DIR, "index_test.bam.csi"), false, null);

src/test/java/htsjdk/samtools/HtsgetBAMFileReaderTest.java

@@ -3,15 +3,16 @@
 import htsjdk.HtsjdkTest;
 import htsjdk.samtools.util.CloseableIterator;
 import org.testng.Assert;
-import org.testng.annotations.AfterTest;
-import org.testng.annotations.BeforeTest;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
 
+@Test(groups = "htsget")
 public class HtsgetBAMFileReaderTest extends HtsjdkTest {
     public static final String HTSGET_ENDPOINT = "http://127.0.0.1:3000/reads/";
     public static final String LOCAL_PREFIX = "htsjdk_test.";
@@ -30,7 +31,7 @@ public class HtsgetBAMFileReaderTest extends HtsjdkTest {
     private static HtsgetBAMFileReader bamFileReaderHtsgetPOST;
     private static HtsgetBAMFileReader bamFileReaderHtsgetAsync;
 
-    @BeforeTest
+    @BeforeMethod
     public void init() throws IOException {
         bamFileReaderHtsgetGET = new HtsgetBAMFileReader(htsgetBAM, true, ValidationStringency.DEFAULT_STRINGENCY, DefaultSAMRecordFactory.getInstance(), false);
         bamFileReaderHtsgetGET.setUsingPOST(false);
@@ -45,7 +46,7 @@ public void init() throws IOException {
         Assert.assertFalse(bamFileReaderHtsgetAsync.isUsingPOST());
     }
 
-    @AfterTest
+    @AfterMethod
     public void tearDown() {
         bamFileReaderHtsgetGET.close();
         bamFileReaderHtsgetPOST.close();