Import properly for tests - #2014
Merged
Merged
Conversation
ModuleGather.getModule loaded each file with load_source() under its bare name
('key' for music21/key.py), which re-executed the file as a separate top-level
module. Every unittest the single-core runner ran therefore tested a shadow
copy of its module: `mod.KeySignature is not music21.key.KeySignature`, and
`isinstance(ks, music21.key.KeySignature)` was False. It also gave those
classes a `__module__` of 'key', so a test asserting a full repr saw
`<key.KeySignature ...>` under CI and `<music21.key.KeySignature ...>` under
pytest -- which is why tests import names from `music21.x` rather than relying
on their own module globals.
Import by fully-qualified name instead, which returns the module music21
already imported. load_source() is now unused and removed.
multiprocessTest was unaffected -- it uses getModuleWithoutImp, which walks the
real package tree.
DO NOT MERGE AS IS -- reported coverage drops from 93.33% to 77.20%.
testSingleCoreAll calls coverageM21.getCoverage() at module import time, after
its own `from music21 import ...` lines have already run, so music21's
module-level code (class bodies, defs, constants) executes before cov.start().
Re-executing every file through load_source() is what currently gets those
lines counted. A real fix has to start coverage before music21 is imported
(measured on the branch: 93.33% at 15c0854, 77.20% with this commit).
Then drop the `from music21.<mod> import ...` lines inside Test methods that
exist only to work around the shadow modules.
AI-assisted (Claude)
Splitting the runner fix from its own consequence: importing modules by package name stopped re-executing them, and reported coverage fell from 93.33% to 77.20%. The re-execution was propping up the number. testSingleCoreAll called coverageM21.getCoverage() at import time, several lines below its own `from music21 import ...` statements, so every class body, def and module constant had already run before cov.start(). Measured directly: with coverage started first, importing music21 alone covers 88/430 statements in key.py and 119/710 in tempo.py; started afterwards, zero. That is the missing 16%. Nothing inside the package can start coverage early, since importing it imports music21. So coverage now wraps the process: `coverage run -m music21.test.testSingleCoreAll ci` on the pinned Python, plain python elsewhere. The omit and exclude lists move to .coveragerc, which the in-process Coverage() was already reading for everything else, and coverageM21.py goes away with the plumbing it existed to hold. Local full run: 93.150% on 3.12, against coveralls' 93.331% on 3.13. Also drops the `from music21 import key` in key.Test.testNonTraditional, which existed only to dodge the shadow modules. AI-assisted (Claude)
sys.argv[1:2] == ['ci'] was a slice used only to dodge IndexError on the no-argument run. Test the length first and the rest reads straight through. Drop unused_returnCode with it; nothing consumed the value. AI-assisted (Claude)
PY_VERSION_WITH_COVERAGE at workflow level; the three step conditions read it from the env context. AI-assisted (Claude)
|
coverage: 93.34% (+0.007%) from 93.333% — test-runner-import into master |
mscuthbert
added a commit
that referenced
this pull request
Aug 28, 2026
coverageM21.py was removed in #2014; the pin is now PY_VERSION_WITH_COVERAGE. AI-assisted (Claude)
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
2 participants
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.
In our tests,
ModuleGather.getModuleused a flaky hack since Python 3. We import modules using our own version of deprecatedimp.load_source()passing in the filename and loading a different memory copy (you might have seen the import warning about order for a long time) -- we tried numerous ways of fixing so we could use import_module instead (56329556b), but it never worked well with Coverage, so we gave up ( f8dd0dd)As
impdisappeared from stdlib we rolled our own (https://github.com/cuthbertLab/music21/pull/1424/changes) in order to still keep coverage. Earlier this week the differences between our system and agent-expected norms came to a head (008dd6b) when the systems had trouble figuring out why, inside key.py, a KeySignature object run under testing had repr<key.KeySignature>unless we didfrom music21.key import KeySignature-- because the test copy ofkeywas different (in address) frommusic21.key. But then of course the agent wanted to change everything --blah blah -- and finally we get here.Claude correctly diagnosed that starting
coverageafter any music21 module had been imported was what lowered coverage there dramatically (93% -> 77%) -- approximately the same swing we saw in 2017("importlib and coverage are not playing well"). Butuv run coverage run -m music21.test.testSingleCoreAll ciworks great. So that's our secret.Removing all music21 tools that deal with coverage. They need to go in .coveragerc which is perfectly fine, and in the CI matrix.
Change the
keytests to not reimport self. Will do other removing "cannot just call Stream() etc." on a downstream commit.