Repository navigation
ENH - GLOWS: Reduce scope of integration tests - #235
Open
leowerneck wants to merge 1 commit into
Open
leowerneck wants to merge 1 commit into
leowerneck wants to merge 1 commit into
Conversation
leowerneck
added
enhancement
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused test-only changes consistently reduce processing scope without affecting production behavior.
0 open findings
What changed in this PR
Reduces GLOWS integration-test runtime by limiting L3e processing to selected repointings.
Changes:
- Adds an L3e repointing filter for integration tests.
- Aligns production-test versions and supports safe reruns.
- Narrows force-reprocessing fixture data to repointing 36.
| File | Description |
|---|---|
tests/integration/test_glows_processor_integration.py |
Limits L3e scope and improves Docker test reruns. |
tests/test_data/glows/imap_glows_force-reprocessing-config_20250101_v000.csv |
Restricts forced processing to repointing 36. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jtniehof
reviewed
Oct 9, 2026
jtniehof
left a comment
jtniehof
left a comment
Collaborator
There was a problem hiding this comment.
Quick look and some quick questions, no deep dive just yet :)
Comment on lines
+148
to
+151
| return patch( | ||
| "imap_l3_processing.glows.l3e.glows_l3e_initializer.identify_versions_for_l3e_output_files", | ||
| side_effect=identify_subset, | ||
| ) |
Collaborator
There was a problem hiding this comment.
The implication I'm drawing is that this is now no longer quite as much of an end-to-end integration test because we're monkey-patching identify_versions_for_l3e_output_files and losing that link of the chain. Am I being a bit paranoid here? Do we separately check the expected set of repointings either here or in a separate unit test?
| l3e_survival-probability-ul-hf repoint36, repoint37, repoint38, repoint39, repoint40, repoint41, repoint42, repoint43, repoint44, repoint45, repoint46, repoint47, repoint48, repoint49, repoint50, repoint51, repoint52, repoint53, repoint54, repoint55, repoint56, repoint57, repoint58, repoint59, repoint60, repoint61, repoint62, repoint63, repoint64, repoint65, repoint66, repoint67, repoint68, repoint69, repoint70, repoint71, repoint72, repoint73, repoint74, repoint75, repoint76, repoint77, repoint78, repoint79, repoint80, repoint81, repoint82, repoint83, repoint84, repoint85, repoint86, repoint87, repoint88, repoint89, repoint90, repoint91, repoint92, repoint93, repoint94, repoint95, repoint96, repoint97, repoint98, repoint99, repoint100, repoint101, repoint102, repoint103, repoint104, repoint105, repoint106, repoint107, repoint108, repoint109, repoint110, repoint111, repoint112, repoint113, repoint114, repoint115, repoint116, repoint117, repoint118, repoint119, repoint120, repoint121, repoint122, repoint123, repoint124, repoint125, repoint126, repoint127, repoint128, repoint129, repoint130, repoint131, repoint132, repoint133, repoint134, repoint135, repoint136, repoint137, repoint138, repoint139, repoint140, repoint141, repoint142, repoint143, repoint144, repoint145, repoint146, repoint147, repoint148, repoint149, repoint150, repoint151, repoint152, repoint153, repoint154, repoint155, repoint156, repoint157, repoint158, repoint159, repoint160, repoint161 | ||
| l3e_survival-probability-ul-sf repoint36, repoint37, repoint38, repoint39, repoint40, repoint41, repoint42, repoint43, repoint44, repoint45, repoint46, repoint47, repoint48, repoint49, repoint50, repoint51, repoint52, repoint53, repoint54, repoint55, repoint56, repoint57, repoint58, repoint59, repoint60, repoint61, repoint62, repoint63, repoint64, repoint65, repoint66, repoint67, repoint68, repoint69, repoint70, repoint71, repoint72, repoint73, repoint74, repoint75, repoint76, repoint77, repoint78, repoint79, repoint80, repoint81, repoint82, repoint83, repoint84, repoint85, repoint86, repoint87, repoint88, repoint89, repoint90, repoint91, repoint92, repoint93, repoint94, repoint95, repoint96, repoint97, repoint98, repoint99, repoint100, repoint101, repoint102, repoint103, repoint104, repoint105, repoint106, repoint107, repoint108, repoint109, repoint110, repoint111, repoint112, repoint113, repoint114, repoint115, repoint116, repoint117, repoint118, repoint119, repoint120, repoint121, repoint122, repoint123, repoint124, repoint125, repoint126, repoint127, repoint128, repoint129, repoint130, repoint131, repoint132, repoint133, repoint134, repoint135, repoint136, repoint137, repoint138, repoint139, repoint140, repoint141, repoint142, repoint143, repoint144, repoint145, repoint146, repoint147, repoint148, repoint149, repoint150, repoint151, repoint152, repoint153, repoint154, repoint155, repoint156, repoint157, repoint158, repoint159, repoint160, repoint161 No newline at end of file | ||
| ignore-after: 2100-01-01 00:00:00+00:00 | ||
| l3e_survival-probability-hi-45 repoint36 |
Collaborator
There was a problem hiding this comment.
Based on the CS principle that the only numbers are zero, one, and many, does it make sense to include two repoints for at least one of the outputs?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Summary
Closes #233.
Overview
Reduce scope of GLOWS integration tests to a small subset of repointings instead of all of them.
A sanity check from @jtniehof will be appreciated. The "get a subset of repointings" was the easiest choice, but maybe this was a poor choice for reasons I'm missing.
File changes
tests/integration/test_glows_processor_integration.pyonly_process_l3e_for()helper to select a subset of repointings.test_run_glows_l3be_against_prodnow uses major version 2, matching prod.test_run_glows_l3be_against_prodoverwrites output, preventing "file already exists" errors on consecutive runs.-e IMAP_API_KEY.tests/test_data/glows/imap_glows_force-reprocessing-config_20250101_v000.csvignore-afterdate in the far future and list only repointing 36 for L3e descriptor.Testing
Running:
results in tests passing in <10 minutes (reduced from >100 minutes).
Related Issues
Will note that further performance considerations might be needed when fixing #234. I will wait to address #234 until this is merged so I know the approach is sensible.