Skip to content

Adding tests to cover the CFEL method's outputs#13

Merged
d-callan merged 8 commits intomainfrom
add-tests
Oct 17, 2025
Merged

Adding tests to cover the CFEL method's outputs#13
d-callan merged 8 commits intomainfrom
add-tests

Conversation

@hverdonk
Copy link
Copy Markdown
Collaborator

@hverdonk hverdonk commented Oct 2, 2025

Summary

New tests

  • Add tests/test_cfel_Beta_and_Qvalue_updates.py:
    • Validates diff_sites from Q-value threshold (≤ 0.20)
    • Validates per-site/group cfel_marker and cfel_beta formatting
    • Confirms expected comparison-site field names from CfelMethod.get_comparison_group_site_fields()
  • Add tests/test_cfel_comparison_outputs.py:
    • Verifies group_N, group_T, group_dN/dS, group_aa_conserved are present in the output csv combined_comparison_summary
    • Asserts presence/types of composition, substitutions, majority_residue
  • Replace old integration test data with new integration test data that includes a substitutions field
    • All other fields in the new integration test data are identical to the fields in the old test data, so the new test data is backwards compatible.
  • Update tests/test_cli.py to ensure deterministic checks (sorting output CSVs to ensure that the first gene in the output matches the first output item checked in the assertions).

Version

  • drhip/version.py bumped to 0.1.3.

@hverdonk hverdonk requested a review from d-callan October 2, 2025 21:31
@d-callan
Copy link
Copy Markdown
Collaborator

d-callan commented Oct 8, 2025

i like this. only thing id like to see is that were still backward compatible to older cfel results json without the subs info. i think all that means for this pr is that rather than replace old tests json files, we add new ones and then write tests conditionally. possibly drhip should produce a warning for older json.

@hverdonk
Copy link
Copy Markdown
Collaborator Author

I like the idea of DRHIP producing a warning for an older json without the 'substitutions' field. All the other json fields are identical between old and new jsons, so backwards compatibility shouldn't be an issue. I'm happy to add old versions of the json test files + conditional tests if you'd prefer, though.

@d-callan
Copy link
Copy Markdown
Collaborator

yea the concern is to make sure we know rather than assume we wont err if were missing certain 'expected' fields. too easy for someone (me, 6 months from now when i dont remember this pr) to modify something that may break this assumption. other fields we may want to err if missing..

@hverdonk
Copy link
Copy Markdown
Collaborator Author

Okay, I've added some results json field validation code. If you like it, I'm happy to merge what we've got into the main branch

@d-callan
Copy link
Copy Markdown
Collaborator

Being explicit about required fields makes me happier, thanks. Looks like we included cfel subs in the required list though? And this doesn't really protect us against the case where someone introduces some logic that will err if the subs field is missing even if it's not explicitly declared required. But merge if you like, and I can always look at it later too.

@d-callan d-callan merged commit 7ecd8ff into main Oct 17, 2025
@d-callan d-callan deleted the add-tests branch October 17, 2025 15:38
@hverdonk
Copy link
Copy Markdown
Collaborator Author

hverdonk commented Oct 17, 2025

A lot of the internal code already checks to see if required fields are present before trying to calculate, for example, majority residue. If a required field isn't present for calculating a given thing, DRHIP warns the user that the required field isn't present in the results json, and that the thing isn't present in the output. For me, this covers the case where we're using old CFEL results that don't have the substitutions field - DRHIP will still produce what output it can, but it will warn that one of the required fields is missing from CFEL and skip a few fields as a result.

If we wanted to be stricter, we could always force DRHIP to quit with an error if a required field isn't present in the input data, or add a --strict command line option where warnings lead to an early termination instead of letting DRHIP analysis continue

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.

2 participants