Skip to content

fix(ptm-tests): Fix unit tests for PTM differential analysis#204

Merged
tonywu1999 merged 1 commit into
develfrom
fix-tests
Apr 27, 2026
Merged

fix(ptm-tests): Fix unit tests for PTM differential analysis#204
tonywu1999 merged 1 commit into
develfrom
fix-tests

Conversation

@tonywu1999

@tonywu1999 tonywu1999 commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Motivation and Context

PTM (post-translational modification) differential analysis in MSstatsShiny uses a specialized data structure that differs from standard protein analysis. When analyzing PTM data, the extract_significant_proteins function returns a list containing three separate data frames: PTM.Model, PROTEIN.Model, and ADJUSTED.Model (the adjusted PTM results accounting for protein-level abundance).

The issue was that unit tests for PTM differential analysis were incorrectly validating the overall structure of the returned object rather than the expected number of significant rows within the ADJUSTED.Model data frame specifically. This misalignment between the test assertion and the actual function behavior prevented accurate verification of PTM result filtering.

Changes Made

  • Test Assertion Fix: Updated the PTM test in test-utils-statmodel-server.R to validate nrow(result$ADJUSTED.Model) instead of nrow(result). The corrected test now properly checks that filtering by adjusted p-value threshold produces exactly 1 significant protein (out of 2 proteins in ADJUSTED.Model where only P1 with p-value 0.04 falls below the 0.05 threshold).

Unit Tests Added or Modified

Modified: test-utils-statmodel-server.R - Test case "extract_significant_proteins filters PTM data correctly"

The test validates that:

  • Input data contains an ADJUSTED.Model data frame with 2 proteins
  • Filtering with significance threshold of 0.05 correctly identifies 1 protein with adjusted p-value below threshold
  • The corrected assertion expect_equal(nrow(result$ADJUSTED.Model), 1) now properly reflects the list-based return structure of the PTM-specific extraction function

@coderabbitai

coderabbitai Bot commented Apr 27, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

A test assertion in the PTM validation is updated to check the row count within the ADJUSTED.Model element of the returned object instead of checking the overall object's row count, aligning with the actual result structure.

Changes

Cohort / File(s) Summary
Test Assertion Update
tests/testthat/test-utils-statmodel-server.R
Modified PTM test to validate significant rows within the ADJUSTED.Model element rather than the overall returned object row count, correcting the assertion target.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A single line hops to the right,
From object's edge to nested sight,
The ADJUSTED.Model now takes the stage,
Where truth was hiding, locked in cage!
One change, so small, yet makes it bright! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing unit tests for PTM differential analysis by correcting test assertions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/testthat/test-utils-statmodel-server.R (1)

272-291: Optional: also assert PTM.Model and PROTEIN.Model filtering.

Since PTM mode returns all three models, consider adding assertions for PTM.Model (expect 2 significant: 0.001, 0.03) and PROTEIN.Model (expect 1 significant: 0.02) to fully cover the PTM branch and guard against regressions in the other two filters.

♻️ Suggested additional assertions
   result <- extract_significant_proteins(data_comp, loadpage_input, 0.05)
   expect_equal(nrow(result$ADJUSTED.Model), 1)
+  expect_equal(nrow(result$PTM.Model), 2)
+  expect_equal(nrow(result$PROTEIN.Model), 1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/testthat/test-utils-statmodel-server.R` around lines 272 - 291, The
test is only asserting ADJUSTED.Model; update the test for
extract_significant_proteins to also assert that result$PTM.Model contains 2
significant rows (adj.pvalue 0.001 and 0.03) and that result$PROTEIN.Model
contains 1 significant row (adj.pvalue 0.02); locate the assertions immediately
after result <- extract_significant_proteins(...) in
tests/testthat/test-utils-statmodel-server.R and add expect_equal checks for
nrow(result$PTM.Model) == 2 and nrow(result$PROTEIN.Model) == 1 (optionally
assert specific protein IDs if desired).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/testthat/test-utils-statmodel-server.R`:
- Around line 272-291: The test is only asserting ADJUSTED.Model; update the
test for extract_significant_proteins to also assert that result$PTM.Model
contains 2 significant rows (adj.pvalue 0.001 and 0.03) and that
result$PROTEIN.Model contains 1 significant row (adj.pvalue 0.02); locate the
assertions immediately after result <- extract_significant_proteins(...) in
tests/testthat/test-utils-statmodel-server.R and add expect_equal checks for
nrow(result$PTM.Model) == 2 and nrow(result$PROTEIN.Model) == 1 (optionally
assert specific protein IDs if desired).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f7063685-ba9b-41cf-83c8-5befb5a67d41

📥 Commits

Reviewing files that changed from the base of the PR and between e225219 and bf0d2ba.

📒 Files selected for processing (1)
  • tests/testthat/test-utils-statmodel-server.R

@tonywu1999 tonywu1999 merged commit 3977d80 into devel Apr 27, 2026
2 checks passed
@tonywu1999 tonywu1999 deleted the fix-tests branch April 27, 2026 17:49
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