Skip to content

feat: test coverage for extension_support/plugins module#3166

Open
Vaishnav88sk wants to merge 3 commits into
Netflix:masterfrom
Vaishnav88sk:feat/add-extension-plugins-tests
Open

feat: test coverage for extension_support/plugins module#3166
Vaishnav88sk wants to merge 3 commits into
Netflix:masterfrom
Vaishnav88sk:feat/add-extension-plugins-tests

Conversation

@Vaishnav88sk
Copy link
Copy Markdown

PR Type

  • Bug fix
  • New feature
  • Core Runtime change (higher bar -- see CONTRIBUTING.md)
  • Docs / tooling
  • Refactoring

Summary

Add unit test coverage for extension_support/plugins.py (15 tests).

Context / Motivation

The extension support plugin system is a critical part of Metaflow's architecture that allows third-party packages to extend Metaflow's functionality. Despite its importance, the plugin utility functions had zero test coverage.

Issue

Fixes #3163

Reproduction

Runtime:

Commands to run:

# paste exact commands

Where evidence shows up:

Before (error / log snippet)
paste here
After (evidence that fix works)
paste here

Root Cause

Why This Fix Is Correct

  • All 15 new tests pass
  • Tests use real Metaflow classes (RetryDecorator) to verify plugin loading
  • Edge cases covered: empty lists, unknown categories, invalid paths

Failure Modes Considered

Changes Made

Added 15 tests covering:

  • merge_lists: List merging with override behavior (5 tests)
  • get_plugin_name: Name extraction for all plugin categories (6 tests)
  • get_plugin: Plugin loading and error handling (2 tests)
  • get_trampoline_cli_names: Trampoline CLI tracking (2 tests)

Tests

  • Unit tests added/updated
  • Reproduction script provided (required for Core Runtime)
  • CI passes
  • If tests are impractical: explain why below and provide manual evidence above

Non-Goals

AI Tool Usage

  • No AI tools were used in this contribution
  • AI tools were used (describe below)
    for formatting

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 29, 2026

Greptile Summary

This PR adds 15 unit tests for extension_support/plugins.py covering merge_lists, get_plugin_name, get_plugin, and get_trampoline_cli_names. Most tests are straightforward and correctly exercise the production code, but a few have assertion gaps that reduce their diagnostic value.

Confidence Score: 4/5

Safe to merge; issues are test quality problems, not production regressions.

All findings are P2 test quality issues. The production code is unchanged. The one inline comment flags a set-based assertion that defeats its own purpose.

test/unit/test_extension_plugins.py — review assertion correctness in TestMergeLists and TestGetTrampolineCliNames.

Important Files Changed

Filename Overview
test/unit/test_extension_plugins.py New test file for extension_support/plugins.py; two tests have correctness problems already flagged in previous review rounds, and one additional test uses set-based deduplication that defeats its own "no duplicates" assertion.

Reviews (3): Last reviewed commit: "Merge branch 'master' into feat/add-exte..." | Re-trigger Greptile

Comment thread test/unit/test_extension_plugins.py
Comment thread test/unit/test_extension_plugins.py
@romain-intel
Copy link
Copy Markdown
Contributor

Great to add tests. Let's address the greptile and test comments and then I'll take a closer look.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@f357565). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3166   +/-   ##
=========================================
  Coverage          ?   27.38%           
=========================================
  Files             ?      375           
  Lines             ?    51746           
  Branches          ?     9143           
=========================================
  Hits              ?    14170           
  Misses            ?    36675           
  Partials          ?      901           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Adds test coverage for merge_lists, get_plugin_name, get_plugin,
and get_trampoline_cli_names functions. Tests verify list merging
behavior, plugin name extraction for various categories, error
handling for invalid plugin paths, and trampoline CLI tracking.
@Vaishnav88sk Vaishnav88sk force-pushed the feat/add-extension-plugins-tests branch from 8a75715 to 7366651 Compare May 1, 2026 19:23
@Vaishnav88sk
Copy link
Copy Markdown
Author

@romain-intel I've addressed the greptile bot's comments:

  1. test_overrides_win_over_base now uses the same name "b" in both base and overrides, and verifies the override instance wins
  2. test_contains_expected_entries now properly mocks _trampoline_cli_names before asserting

All 15 tests now pass with correct assertions. The branch has been force-pushed with the fixes.

@Vaishnav88sk Vaishnav88sk force-pushed the feat/add-extension-plugins-tests branch from 4bf16fb to 7366651 Compare May 1, 2026 19:39
@Vaishnav88sk
Copy link
Copy Markdown
Author

@romain-intel

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.

Add test coverage for extension_support/plugins module

2 participants