Skip to content

fix: enable test suite and fix broken build dependencies#3

Merged
ypriverol merged 5 commits intodevfrom
fix/test-infrastructure
Apr 13, 2026
Merged

fix: enable test suite and fix broken build dependencies#3
ypriverol merged 5 commits intodevfrom
fix/test-infrastructure

Conversation

@ypriverol
Copy link
Copy Markdown
Member

@ypriverol ypriverol commented Apr 13, 2026

Summary

  • Remove skipTests=true from pom.xml — tests now run by default with mvn test
  • Update maven-compiler-plugin source/target from Java 1.8 to Java 17 (matches runtime)
  • Add missing compile dependencies that master code references but were never declared:
    • jmzml 1.7.11 (used by MzMLAdapter, MzMLSpectraMap, SpectrumConverter, MzMLSpectraIterator)
    • fastutil 8.5.12 (required by jmzml at runtime)
    • slf4j-api 1.7.36 + logback-classic 1.2.12 (logging, required by jmzml)
    • commons-io 2.15.1 (used by BuildSA, ConcurrentMSGFPlus)
  • @Ignore TestMzML test that requires Windows-specific DMS files (C:\DMS_WorkDir1)

Why

Master was previously un-compilable without these dependencies in the local Maven cache. Tests were globally disabled via skipTests=true, masking the broken build. This PR fixes the foundation so that all subsequent PRs can gate on mvn test.

Test results

Tests run: 120, Failures: 0, Errors: 0, Skipped: 67
BUILD SUCCESS

53 active tests pass. 67 are pre-existing @Ignore (require external data files).

Test plan

  • mvn compile succeeds (was failing before)
  • mvn test passes with 0 failures
  • mvn package produces a working JAR
  • Smoke test: run JAR with test.mgf + BSA.fasta

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Build & Infrastructure

    • Upgraded Java target to 17, enabled tests during build, and added runtime/logging libraries.
  • New Features

    • Introduced an MS-level range option to restrict which spectra are processed (configurable).
  • Bug Fixes

    • Improved robustness of mzIdentML generation to skip missing spectra and continue evaluating matches.
  • Tests

    • Added several unit/integration tests for MS-level parsing and mzIdentML output; one platform-specific test marked ignored.

…Java 17

- Change skipTests from true to false so mvn test actually runs
- Update maven-compiler-plugin source/target from 1.8 to 17 (matches runtime)
- Add missing compile dependencies: jmzml 1.7.11, fastutil 8.5.12,
  slf4j-api 1.7.36, logback-classic 1.2.12, commons-io 2.15.1
  (master code references these classes but they were not declared)
- @ignore TestMzML test that requires Windows-specific DMS files

Result: 120 tests run, 53 active, 67 skipped, 0 failures

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 13, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6b84c635-1980-4834-b04e-4c0b7c6cb800

📥 Commits

Reviewing files that changed from the base of the PR and between 191ba71 and 5f0ced1.

📒 Files selected for processing (15)
  • pom.xml
  • src/main/java/edu/ucsd/msjava/msdbsearch/SearchParams.java
  • src/main/java/edu/ucsd/msjava/msutil/SpecKey.java
  • src/main/java/edu/ucsd/msjava/msutil/SpectraAccessor.java
  • src/main/java/edu/ucsd/msjava/mzid/MZIdentMLGen.java
  • src/main/java/edu/ucsd/msjava/mzml/MzMLAdapter.java
  • src/main/java/edu/ucsd/msjava/params/IntRangeParameter.java
  • src/main/java/edu/ucsd/msjava/params/ParamManager.java
  • src/main/java/edu/ucsd/msjava/parser/MzXMLSpectraMap.java
  • src/main/java/edu/ucsd/msjava/ui/MSGFDB.java
  • src/main/java/edu/ucsd/msjava/ui/MSGFDBLib.java
  • src/main/java/edu/ucsd/msjava/ui/MSGFPlus.java
  • src/test/java/msgfplus/TestIntRangeParameter.java
  • src/test/java/msgfplus/TestMSLevelFiltering.java
  • src/test/java/msgfplus/TestMZIdentMLGen.java

📝 Walkthrough

Walkthrough

Adds configurable MS-level filtering end-to-end (parameter, parsing, accessor, spec key generation, and callers), updates Maven to run tests and target Java 17, adjusts MS-level inclusivity semantics in some adapters, and expands test coverage for MS-level and mzIdentML generation; one Windows-specific test is annotated @Ignore.

Changes

Cohort / File(s) Summary
Build Configuration & Dependencies
pom.xml
Enabled test execution (skipTests=false), upgraded maven-compiler source/target from 1.817, removed a dedicated fastutil shade filter, and added dependencies: jmzml, fastutil, slf4j-api, logback-classic, commons-io.
MS-level parameter & parsing
src/main/java/edu/ucsd/msjava/params/ParamManager.java, src/main/java/edu/ucsd/msjava/params/IntRangeParameter.java
Added msLevel parameter to ParamManager (default 2,2) with accessor; IntRangeParameter.parse now accepts single-value and two-value ranges and fixes an error message typo.
Search parameters & propagation
src/main/java/edu/ucsd/msjava/msdbsearch/SearchParams.java
Added minMSLevel and maxMSLevel fields, getters, parsing integration, and included MSLevel in toString().
Spectra access & filtering
src/main/java/edu/ucsd/msjava/msutil/SpectraAccessor.java, src/main/java/edu/ucsd/msjava/msutil/SpecKey.java
SpectraAccessor gains setMSLevelRange(min,max) and applies msLevel bounds to MZXML/MZML adapters/iterators; SpecKey.getSpecKeyList overloads accept min/max MS level and filter spectra by MS level (counts filtered).
Caller updates
src/main/java/edu/ucsd/msjava/ui/MSGFPlus.java, src/main/java/edu/ucsd/msjava/ui/MSGFDB.java, src/main/java/edu/ucsd/msjava/ui/MSGFDBLib.java
Call sites updated to pass MS-level bounds into SpectraAccessor and SpecKey.getSpecKeyList (defaults used where applicable).
Adapter/internals MS-level semantics
src/main/java/edu/ucsd/msjava/mzml/MzMLAdapter.java, src/main/java/edu/ucsd/msjava/parser/MzXMLSpectraMap.java
Comments and conditional checks adjusted to treat maxMSLevel as inclusive in several places (some internal consistency fixes applied).
MZIdentML generation robustness
src/main/java/edu/ucsd/msjava/mzid/MZIdentMLGen.java
Skip null Spectrum with warning when missing; change to continue instead of break when DeNovoScore check fails to allow earlier matches to be evaluated.
Tests added/modified
src/test/java/msgfplus/TestMSGFPlus.java, src/test/java/msgfplus/TestIntRangeParameter.java, src/test/java/msgfplus/TestMSLevelFiltering.java, src/test/java/msgfplus/TestMZIdentMLGen.java
Marked Windows-specific test method with @Ignore; added unit tests for IntRangeParameter and MS-level parameter parsing; added integration-style mzIdentML generation tests validating score CVParams and basic mzIdentML structure.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI as MSGFPlus (CLI)
    participant Params as ParamManager
    participant Search as SearchParams
    participant Accessor as SpectraAccessor
    participant SpecKey
    participant SpectrumSource as MZML/MZXML Adapter/Iterator

    User->>CLI: run with -msLevel or default
    CLI->>Params: addMSGFPlusParams / getMSLevelParameter
    CLI->>Search: parse params -> minMSLevel,maxMSLevel
    CLI->>Accessor: setMSLevelRange(min,max)
    CLI->>SpecKey: getSpecKeyList(..., min,max)
    SpecKey->>SpectrumSource: iterate spectra
    SpectrumSource-->>SpecKey: yield Spectrum (only if msLevel in [min,max])
    SpecKey-->>CLI: return filtered SpecKey list
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hop through params, min to max in tow,
New MS bounds snug where spectra go,
Java seventeen hops in with glee,
Tests and mzid checks — hoppity, we see! 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main objectives: enabling the test suite (by setting skipTests to false) and fixing broken build dependencies (by adding five missing dependencies and updating Java version).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/test-infrastructure

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 and usage tips.

ypriverol and others added 4 commits April 13, 2026 08:00
…shold

In MZIdentMLGen.addSpectrumIdentificationResults(), change `break` to
`continue` when a match has DeNovoScore below the minimum threshold.
The `break` was incorrectly stopping emission of all subsequent matches
for that spectrum, silently dropping valid PSMs from the mzid output.

Also add null safety check for spectrum index lookup — if a spectrum
index is not found in the spectrum file, log a warning and skip
instead of throwing a NullPointerException.

Add TestMZIdentMLGen with two integration tests:
- testMzidScoreCompleteness: runs MSGF+ search, verifies every SII
  has all 4 score CVParams (RawScore, DeNovoScore, SpecEValue, EValue)
- testMzidStructuralValidity: verifies output mzid has required
  mzIdentML structure elements

Closes MSGFPlus#157

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add new -msLevel CLI parameter to filter spectra by MS level.
Accepts single value (e.g., -msLevel 2) or comma-separated range
(e.g., -msLevel 2,3). Default is 2 (MS2 only).

Changes:
- ParamManager: add MS_LEVEL enum and registration
- IntRangeParameter: enable single-value parsing, fix typo
- SearchParams: add minMSLevel/maxMSLevel fields
- SpecKey: filter spectra by MS level in getSpecKeyList()
- SpectraAccessor: add setMSLevelRange(), wire to parsers
- MzMLAdapter/MzXMLSpectraMap: fix maxMSLevel to be inclusive
- MSGFPlus/MSGFDB/MSGFDBLib: wire MS level parameters
- pom.xml: remove fastutil shade filter (jmzml 1.7.11 needs full fastutil)

Tests: TestIntRangeParameter (9 tests), TestMSLevelFiltering (6 tests)

Benchmark (TMT 1.1GB, TDA):
  Baseline: 1245s, 6654 PSMs@1%FDR
  -msLevel 2: 957s (-23%), 6936 PSMs@1%FDR (+4.2%)

Closes MSGFPlus#159

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
feat(MSGFPlus#159): add -msLevel parameter for MS level filtering
fix(MSGFPlus#157): preserve PSM scores when DeNovoScore is below threshold
@ypriverol ypriverol changed the base branch from master to dev April 13, 2026 09:35
@ypriverol ypriverol merged commit 4f74816 into dev Apr 13, 2026
1 check was pending
@ypriverol ypriverol deleted the fix/test-infrastructure branch April 16, 2026 16:36
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.

1 participant