Remove the hardcoded data paths from the scripts and their tests - #10
Open
emrefbulut wants to merge 1 commit into
Open
Remove the hardcoded data paths from the scripts and their tests#10emrefbulut wants to merge 1 commit into
emrefbulut wants to merge 1 commit into
Conversation
scripts/leakage_real.py, scripts/leakage_loraiq.py, tests/test_preflight.py and tests/test_measurement.py all fell back to an absolute path inside one developer's temporary directory. It resolved on exactly one machine, so the scripts ran there and were unrunnable everywhere else, and the LoRaIQ tests passed there and skipped everywhere else -- while docs/methodology.md opens with "everything below is reproducible from this repository". The fallback is the part that made it invisible. Without one, an absent dataset is an error the first person to run the script sees. With one, the author never sees anything and everyone else sees a skip that reads like a property of their machine rather than of the repository. The datasets are now named by IQFORGE_DASH7 and IQFORGE_LORAIQ, with IQFORGE_LORAIQ_INDEX / _LABELS / _GROUPS optional because the CSVs are looked up beside the recordings. No fallback. Every path keeps the command-line flag it already had, and the flags still win. The scripts stop with the variable name in the message; the tests skip with the variable name and distinguish "never set" from "set to something that is not a directory", because those need opposite fixes. The two test files carried the same fifteen-line resolver twice; it now lives once in tests/helpers.py. tests/test_repo_hygiene.py is new and scans src/, scripts/ and tests/ for committed machine paths, so this cannot come back quietly -- convention 8 is the one with no natural failure mode, since the mistake works perfectly for whoever makes it. A line that only looks like a machine path opts out with an inline not-a-machine-path marker, which puts the exemption where a reviewer reads it rather than in a file allowlist. One line qualifies today: synthetic overflow data in the audit width test. docs/methodology.md's Reproducing section now shows the variables, which is what makes the opening claim true rather than aspirational.
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.
What changed
Four files carried an absolute path into one developer's temporary directory as a fallback:
scripts/leakage_real.py— DASH7 sourcescripts/leakage_loraiq.py— LoRaIQ source + three CSVstests/test_preflight.py— LoRaIQ resolvertests/test_measurement.py— the same resolver, duplicatedAll four now read
IQFORGE_DASH7/IQFORGE_LORAIQ(plus optionalIQFORGE_LORAIQ_INDEX,_LABELS,_GROUPS) with no fallback. Every path keeps the command-line flag it already had, and the flags still win.The duplicated fifteen-line resolver now lives once in
tests/helpers.py.docs/methodology.md§Reproducing shows the variables.tests/test_repo_hygiene.pyis new.Why
The path resolved on exactly one machine. The scripts ran there and were unrunnable everywhere else; the LoRaIQ tests passed there and skipped everywhere else — while
docs/methodology.mdopens with "Everything below is reproducible from this repository."The fallback is the part that made it invisible. Without one, an absent dataset is an error the first person to run the script sees immediately. With one, the author never sees anything at all, and everyone else sees a skip that reads like a fact about their machine rather than about the repository.
The skip reason mattered too.
"LoRaIQ recordings are not on this machine"cannot be acted on: it does not distinguish you never configured this from you configured it and mistyped the path, and those need opposite fixes. Now:and for a set-but-wrong path,
IQFORGE_LORAIQ is set to '...', which is not a directory.How it was verified
Control — the tests genuinely run when the variable is set, not just skip more politely:
Three mutations, each red on two tests — the mechanical scan and the behavioural one:
test_no_source_file_hardcodes_a_machine_path[tests/helpers.py]+test_the_resolver_finds_nothing_when_the_variable_is_unsettest_the_loraiq_skip_reason_names_the_variable_to_set+test_the_resolver_finds_nothing_when_the_variable_is_unsetleakage_loraiq.pya default path againtest_no_source_file_hardcodes_a_machine_path[scripts/leakage_loraiq.py]+test_the_experiment_scripts_expose_no_default_data_pathThe first mutation is the original defect, restored verbatim. It is caught.
The new hygiene test
Convention 8 is the one with no natural failure mode — the mistake works perfectly for whoever makes it, which is why it survived four files and a documented claim. So the check is mechanical:
tests/test_repo_hygiene.pyscanssrc/,scripts/andtests/for string literals anchored at a Windows drive letter or a POSIX home/tmp root, one parametrised case per file so the failure names the file.A line that only looks like a machine path opts out with an inline
not-a-machine-pathmarker rather than a file-level allowlist — the exemption then sits where a reviewer reads it, next to the string it excuses. Exactly one line qualifies today: synthetic overflow data in the audit width test ("C:/a-very/deep/path/" * 8).Note for the reviewer
Two files beyond the ones named in the task had the identical defect:
tests/test_measurement.py(same duplicated resolver) andscripts/leakage_real.py(DASH7 source). Fixing only the two named ones would have left the hygiene test failing on the other two, and left--sweep strideon DASH7 unrunnable. Say if you would rather split them out.Conventions it touches