Add tests for the catalog tooling - #183
Open
vietnamesekid wants to merge 1 commit into
Open
vietnamesekid wants to merge 1 commit into
vietnamesekid wants to merge 1 commit into
Conversation
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 this PR does
Not a plugin submission, so the plugin and ownership checklist in the template does not apply.
This adds the first tests in the repo:
scripts/test_catalog_tools.py, plus one CI step that runs them.#173 offered to add
scripts/test_validate_catalog.pyif you wanted it ("just say the word"). This is that file. I widened it a bit to also cover the path containment and parsing helpers inplugin_catalog.py, since those have the same problem.It is independent of #173. No file overlap, merges cleanly in either order, and it deliberately does not assert the source shapes that #173 changes. More on that below.
Why
validate-catalog.pyis the supply chain gate. Its own docstring says what is at stake:plugin_catalog.pyis the other half of it. It fetches arbitrary third party repos at index generation time and guards them withresolve_inside/containedplus a few size caps.Neither has a single test. Right now every rule in that docstring holds only because the code happens to still be correct, and nothing would tell you if that stopped being true.
What it covers
52 cases, stdlib
unittestonly. No new dependencies, no network, no git subprocesses, about 0.02s.TestShaPinningre.IGNORECASEorMULTILINEregressionTestSourcePathContainmentpathescaping via.., absolute paths, backslash traversal, empty segmentsTestLocalSourcesTestLiveCatalogTestPathHelpersresolve_inside/contained/plugin_root_for_fetch, including absolute component replacement and symlink escapeTestCleanplugin-index.jsonTestFrontmatter...terminatorTestLoadJsonFileTestManifestVersionisinstance(True, int)turning"version": trueinto the string"True"TestReplaceShaInCatalogEach class has a docstring naming the threat it covers, so the file also reads as a description of what the gate is actually for.
One thing I found but did not fix here
SHA_REaccepts a sha with a trailing newline, so"<40 hex>\n"passes. Python's$also matches right before a trailing newline. A pin like that gets through validation but can never matchgit rev-parse HEADat install time, so it fails late instead of failing in CI. Same thing inplugin_catalog.SHA_RE, which is what the daily bump bot uses.I left it as an
@unittest.expectedFailureso the suite stays green and the gap is still written down somewhere. The fix is\Zinstead of$, or.fullmatch, in both places. I kept it out of a tests-only PR on purpose. Happy to send it separately, or fold it in here if you would rather have it in one go.What I deliberately did not assert
Shapes like
{"source": "github", "repo": ...}, bare string URLs,null, numbers and arrays currently return no errors, and #173 changes them to be rejected.If I asserted today's behavior, this suite would become the thing blocking that fix. So those cases are left out, with a comment in the file pointing at #173. Worth adding once it lands.
Verification
Green against unmodified
main. This PR does not touch any source file.Paths come from
Path(__file__).resolve().parent, so the suite does not care about cwd. I checked it from the repo root, fromscripts/, and by running the file directly.I also wanted to be sure the tests actually fail when something breaks, rather than just passing. So I broke the source on purpose and checked:
SHA_REgainsre.IGNORECASEresolve_insideboolguard removed frommanifest_versionload_json_fileAll reverted afterwards,
git diffagainst the scripts is empty.That is also how I found the trailing newline thing above.
CI
One step appended to the existing workflow:
-s scriptsscopes discovery so it can never pick up a test file from a vendored plugin underexternal_plugins/.CONTRIBUTING.mdand the PR template both promise to list exactly what CI runs, so they each get the same one line.Notes for reviewers
unittest, zero dependencies, one file, one CI line, under a second. Nothing imports it.validate-catalog.pyandbump-plugin-shas.pyare hyphenated so they cannot be imported by name. I load them withimportlib.util.spec_from_file_location. Both guard their entry point withif __name__ == "__main__", so importing them does not runmain().TestReplaceShaInCatalogpatches the module levelREPO_ROOTbecause that function has no injection point. I noted it in the docstring.