tests(changelog): rewrite get_data_from_entries coverage to ALL-and-ONLY mock style#4517
Closed
Copilot wants to merge 2 commits into
Closed
Conversation
Agent-Logs-Url: https://github.com/envoyproxy/toolshed/sessions/f3b35da4-8770-48d1-9bbc-1dfc1516f191 Co-authored-by: phlax <454682+phlax@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor new tests for changelog to use mock testing style
tests(changelog): rewrite May 14, 2026
get_data_from_entries coverage to ALL-and-ONLY mock style
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.
This refines the
AChangelog.get_data_from_entriestest additions from PR #4501 by removing real filesystem-based tests and aligning them with the file’s established mock-only unit-test convention. Production behavior remains unchanged; only test strategy/style is updated.Reworked 7
get_data_from_entriestests to mock-onlytmp_path/ real dirs / real file I/O withMagicMockpaths andentry_dir.glob(...)stubs.parent.name,stem,read_text) while keepingstemas real strings so.count()/.split()execute naturally.Aligned assertions with canonical “ALL and ONLY” style
utils.from_yamltyping(typing.Change, dict type aliases)castsorted(to explicitly lock ordering semantics)call_args/call_args_listassertions for success and failure paths.Preserved branch/behavior coverage from the original intent
no_separator,a__b__c) withChangelogParseErrormessage checks including path interpolation.utils.from_yaml.side_effect(propagate generic errors / wrap known parse/type-cast errors).Original prompt
Context
This refines PR #4501 (branch
copilot/add-per-entry-rst-directory-reader), which addsAChangelog.get_data_from_entriesfor the per-entry changelog layout.The implementation in
py/envoy.base.utils/envoy/base/utils/abstract/project/changelog.pyis fine and must not change in this PR.The tests added in
py/envoy.base.utils/tests/test_abstract_project_changelogs.pyuse real filesystem (tmp_path) with real.write_text/.glob/.read_textcalls. That does not match the project's testing conventions — the rest of this file is exclusively mock-based, "ALL and ONLY" style unit testing (see existingtest_abstract_changelog_get_dataimmediately above the new tests as the canonical example: it patchescast,utils.from_yaml,typingvia thepatchesfixture and asserts oncall_argsexactly).This PR rewrites the new tests to that style. No production code changes.
What to do
Rewrite the seven new test functions to be fully mocked
Currently in
py/envoy.base.utils/tests/test_abstract_project_changelogs.py(added by PR #4501):test_abstract_changelog_get_data_from_entries_happy_pathtest_abstract_changelog_get_data_from_entries_arbitrary_sectiontest_abstract_changelog_get_data_from_entries_missing_separatortest_abstract_changelog_get_data_from_entries_multiple_separatorstest_abstract_changelog_get_data_from_entries_missing_yamltest_abstract_changelog_get_data_from_entries_empty_dirtest_abstract_changelog_get_data_from_entries_stable_orderingAll of these currently use
tmp_path, real directories, real files. Rewrite them all so that:No real filesystem. No
tmp_path, no.mkdir(), no.write_text(), no.read_text()against real paths. Everything viaMagicMock/ thepatchesfixture.Patch the same module-level dependencies the existing
test_abstract_changelog_get_datapatches, namely (where relevant):castutils.from_yamltypingexceptionsis already imported at module level in the tests file, so you can either patch it or use the real one; match the convention oftest_abstract_changelog_get_datawhich uses the realexceptions.ChangelogParseErrorto assert on.Mock
entry_dir.glob(...)to return a deterministic iterable of mocked path objects. Each mocked path needs:path.parent.name→ the section namepath.stem→ thearea__slug(or whatever malformed variant the test exercises)path.read_text.return_value→ the change textpath.split(...)etc. is NOT needed — the production code callspath.stem.split(ENTRY_SEPARATOR, 1), so setpath.stemto a real string (e.g."oauth2__foo_fix") so that.split/.countwork naturally on it. Stems are strings; you do not need to mock.split/.countseparately.Assert on
call_args/call_args_listexactly the waytest_abstract_changelog_get_datadoes — e.g. assert thatutils.from_yamlwas called with(yaml_path, m_typing.ChangelogSourceDict), assert thatcastwas called with(m_typing.ChangelogDict, <expected dict>), assert thattyping.Changewas called for each entry's content, etc. The point of "ALL and ONLY" is that every observable call is asserted and nothing else happens.Use
iters/ parametrisation where it reduces duplication, matching the patterns already in this file. Not mandatory if it makes a single-case test harder to read; use judgement consistent with surrounding tests.yaml_pathandentry_dirshould beMagicMock()instances passed directly intoabstract.AChangelog.get_data_from_entries(yaml_path, entry_dir). Do not construct realpathlib.Pathobjects.For the "missing yaml" case (
test_abstract_changelog_get_data_from_entries_missing_yaml): the current implementation letsFileNotFoundErrorpropagate raw fromutils.from_yaml. Mirror the style oftest_abstract_changelog_get_datawhich parametrises raises across[None, Exception, yaml.reader.ReaderError, exceptions.TypeCastingError]. Either:get_data_from_entriescovering the same set of raise classes fromutils.from_yamland the same two-branch behaviour (re-raise asChangelogParseErrorfor known errors, propagateExceptionraw), ORThe existing PR's
test_..._missing_yamlshould be replaced by tests that mockutils.from_yaml.side_effectrather than relying on a real missing file.For the "missing separator" / "multiple separators" cases: drive these by setting
path.stemto"no_separator"/"a__b__c"on a mocked path returned by the mockedglob. Assertexceptions.ChangelogParseErroris raised and the path's string repr (or whatever the production code interpolates — currentlyf"...: {path}") is in the message; sincepathis aMagicMock...This pull request was created from Copilot chat.