Skip to content

[Updater] Ensure we no longer test the legacy code path#7136

Merged
brrygrdn merged 6 commits intomainfrom
brrygrdn/avoid-testing-legacy-paths
Apr 24, 2023
Merged

[Updater] Ensure we no longer test the legacy code path#7136
brrygrdn merged 6 commits intomainfrom
brrygrdn/avoid-testing-legacy-paths

Conversation

@brrygrdn
Copy link
Copy Markdown
Contributor

Follows on from #7128

Now that all well-formed job definitions are routed to an Operation class, the only jobs that can possibly hit the Updater#legacy_run path are those where the attributes do not match real-world configurations.

This PR makes a change to disable the Updater#legacy_run method in our test suite so any existing or new tests which hit the legacy code line will fail.

Having done this, it then addresses two cohorts of failing tests:

  • A set of ignore condition tests which are creating new Version PRs but are using job.dependencies as a convenience in their test setup to avoid having to account for N dependencies
    • This fix for these is to adjust the expectations so they only examine the target dependency without using this hook
  • A set of tests which target creating a new PR for a specific dependency that were not security updates
    • For this group the tests were mostly checking the same behaviour for updating as for grouping, so I updated them to be security updates to reflect real world conditions and they continue to pass
    • In one case, testing that a nominated subdependency would still result in an update, I moved this into the "Updating a PR" test as this is the scenario we are most likely to encounter and handle this problem
    • Finally, one test outright checks we can create a fresh PR for a nominated single Dependency, which is behaviour we do not actually use in the real world. I've allowed this to continue passing by enabling the legacy code, but it will be removed when we clean up.

Rationale

The overall goal of the Operation classes is to only have concrete code lines for the job configurations we run in the real world, this ultimately means removing a single capability expressed in our tests at present which is to create a fresh Version Update PR for a set of specific dependencies.

This capability definitely seems like something we might want in future, but in keeping with our intent to make the code paths as explicit as possible I think it makes sense to remove it until we need it rather than leave ambiguity.

As a follow-on from the grouped update introduction and as part of the cleanup from this task the updater_spec should be broken up to more thoroughly test the concrete classes which will make it much easier to bring this back with exhaustive testing rather than having one example in the file currently touch this path.

@brrygrdn brrygrdn requested a review from a team as a April 21, 2023 12:08
@brrygrdn brrygrdn changed the title Ensure we no longer test the legacy code path [Updater] Ensure we no longer test the legacy code path Apr 21, 2023
@brrygrdn brrygrdn force-pushed the brrygrdn/avoid-testing-legacy-paths branch 3 times, most recently from 1a349fd to 99ef87a Compare April 21, 2023 13:57
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other day I was wondering why we had this exception. Nice use of it!

Copy link
Copy Markdown
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense!

@brrygrdn brrygrdn force-pushed the brrygrdn/extract-refresh-security-update-operation branch from d5fe1ee to cd69bf4 Compare April 24, 2023 12:22
Base automatically changed from brrygrdn/extract-refresh-security-update-operation to main April 24, 2023 15:17
@brrygrdn brrygrdn force-pushed the brrygrdn/avoid-testing-legacy-paths branch 5 times, most recently from cb0d338 to 3cfe75f Compare April 24, 2023 15:59
@brrygrdn brrygrdn force-pushed the brrygrdn/avoid-testing-legacy-paths branch from 3cfe75f to 7d10554 Compare April 24, 2023 16:02
@brrygrdn brrygrdn merged commit ed33704 into main Apr 24, 2023
@brrygrdn brrygrdn deleted the brrygrdn/avoid-testing-legacy-paths branch April 24, 2023 16:39
@pavera pavera mentioned this pull request Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants