Skip to content

refactor(tests): ux tests to follow test rules#3245

Open
agsaru wants to merge 8 commits into
Netflix:masterfrom
agsaru:refactor-pytest-ux
Open

refactor(tests): ux tests to follow test rules#3245
agsaru wants to merge 8 commits into
Netflix:masterfrom
agsaru:refactor-pytest-ux

Conversation

@agsaru

@agsaru agsaru commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

PR Type

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

Summary

Copilot AI review requested due to automatic review settings June 8, 2026 18:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR refactors the UX core test suite to reduce duplication and improve consistency by consolidating many per-flow tests into parametrized tests with shared helpers.

Changes:

  • Consolidates multiple test cases into pytest.mark.parametrize patterns across basic/DAG/decorators/config/resume/compliance/compilation suites.
  • Introduces shared helper functions (assertion callbacks and deployment/run helpers) to reduce repeated boilerplate.
  • Improves docstrings, typing annotations, and minor formatting for readability.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
test/ux/core/test_utils.py Normalizes scheduler args and updates run-tracking helper to use wait_for_runs_by_tags.
test/ux/core/test_sfn_compilation.py Parametrizes “standard” SFN compilation tests and cleans up imports/structure.
test/ux/core/test_resume.py Consolidates resume tests with a helper and parametrized scenarios.
test/ux/core/test_lifecycle.py Moves markers to module scope and adds typing annotations.
test/ux/core/test_decorators.py Refactors decorator tests into assertion callbacks + a single parametrized runner.
test/ux/core/test_dag.py Consolidates DAG topology tests into one parametrized test + assertion callbacks.
test/ux/core/test_config.py Refactors config tests into parametrized cases for defaults/overrides/corner cases.
test/ux/core/test_compliance.py Introduces _deploy_and_run_compliance helper and consolidates several tests.
test/ux/core/test_basic.py Consolidates basic flow tests + adds parametrized “custom endpoints” coverage.
test/ux/core/test_argo_compilation.py Adds shared helpers and combines Argo compilation checks into a parametrized test.
test/ux/core/test_airflow_compilation.py Replaces custom tempfile/env handling with fixtures and parametrized compilation tests.
test/ux/core/conftest.py Minor formatting change in devstack env setup.
test/ux/conftest.py Small formatting improvements and modernized string formatting.
Comments suppressed due to low confidence (1)

test/ux/core/test_argo_compilation.py:50

  • test_argo_only_json_exposes_workflow_template appears to have had its core assertions/body removed in this refactor, so it can pass without validating anything (when not skipped). Either delete this now-redundant test (since test_argo_compilation_behaviors covers the same case), or restore the compilation + assertions inside it.
def test_argo_only_json_exposes_workflow_template(

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/ux/core/test_basic.py Outdated
Comment thread test/ux/core/test_argo_compilation.py Outdated
Comment thread test/ux/core/test_resume.py Outdated
Comment thread test/ux/core/test_resume.py
Comment thread test/ux/core/test_resume.py
Comment thread test/ux/core/test_compliance.py Outdated
@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR refactors 11 UX test files to consolidate repetitive standalone test functions into parametrized equivalents, extracting shared assertion callbacks and boilerplate helpers (_deploy_and_run_compliance, _trigger_and_wait). The changes do not alter production code.

  • Several previously-noted issues are addressed: tl_args_extra.pop(\"env\") now operates on a copy, resources_cpu is marked via pytest.param(marks=…) at collection time, and each test_resume_basic_flow variant now gets a unique deployment tag via request.node.callspec.id.
  • test_airflow_compilation.py gains safer env-var setup through monkeypatch.setenv, and tmp_path replaces manual tempfile management.
  • The _assert_deduplicated_task_names callback silently ignores its deployed_flow_name parameter, and the unified test_dag_behaviors now skips "not supported" errors for nested-foreach in runner mode where the old per-test code would have failed.

Confidence Score: 5/5

Test-only refactoring; no production code changes. All assertions from the original tests are accounted for in the new parametrized forms.

The refactoring correctly preserves test semantics across all files. Both findings are minor: an unused parameter in a private callback function, and a widened skip guard for nested-foreach that could mask a future runner-mode regression. Neither affects currently passing tests.

test/ux/core/test_dag.py and test/ux/core/test_argo_compilation.py have minor issues but nothing blocking.

Important Files Changed

Filename Overview
test/ux/conftest.py Minor cleanup: import reordering (stdlib before third-party), % format strings replaced with f-strings, cosmetic blank lines added.
test/ux/core/conftest.py Trivial cosmetic change: added a blank line before a comment block; no logic changes.
test/ux/core/test_airflow_compilation.py Compilation helper moved into the compile_and_validate fixture using tmp_path/monkeypatch; six individual test functions collapsed into a single parametrized test_airflow_dag_compilation.
test/ux/core/test_argo_compilation.py Two standalone tests merged into test_argo_compilation_behaviors; _assert_deduplicated_task_names accepts deployed_flow_name but never uses it (unused parameter).
test/ux/core/test_basic.py Multiple standalone test functions collapsed into three parametrized tests; resources_cpu mark correctly placed as marks=pytest.mark.scheduler_only in pytest.param (collection-time). No functional regressions detected.
test/ux/core/test_compliance.py Boilerplate extracted into _deploy_and_run_compliance helper; tl_args_extra is now copied before pop("env"), fixing the caller-mutation issue noted in the previous review.
test/ux/core/test_config.py Three config test variants collapsed into test_config_simple_behaviors; two mutable-flow variants into test_mutable_flow_behaviors. All assertions from the original tests are preserved.
test/ux/core/test_dag.py All DAG topology tests merged into test_dag_behaviors with allow_unsupported flag. Skip-on-unsupported semantics silently broadened for nested_foreach: old code guarded by exec_mode == deployer, new code skips in all exec modes.
test/ux/core/test_decorators.py Three decorator tests collapsed into test_decorator_behaviors; pytestmark now includes both decorators and basic markers at module level, removing per-test duplicate marker stacking.
test/ux/core/test_lifecycle.py Per-test duplicate marker stacking removed; type annotations added to test signatures; parametrize id strings made more descriptive.
test/ux/core/test_resume.py Three separate resume tests collapsed into test_resume_basic_flow; deployment tag now includes request.node.callspec.id suffix ensuring variant isolation. _trigger_and_wait helper raises TimeoutError on timeout instead of silently continuing.
test/ux/core/test_sfn_compilation.py Seven individual SFN compilation tests collapsed into one parametrized test_standard_flow_compiles_to_valid_asl; imports moved to module level; no logic changes.
test/ux/core/test_utils.py Cosmetic changes only: blank lines added for readability, track_runs_by_tags renamed to wait_for_runs_by_tags in a docstring/call site to match the actual function name.

Reviews (3): Last reviewed commit: "removed extra param" | Re-trigger Greptile

Comment thread test/ux/core/test_argo_compilation.py Outdated
Comment thread test/ux/core/test_basic.py Outdated
Comment thread test/ux/core/test_resume.py
@agsaru agsaru marked this pull request as draft June 8, 2026 19:46
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3245   +/-   ##
=========================================
  Coverage          ?   28.82%           
=========================================
  Files             ?      381           
  Lines             ?    52467           
  Branches          ?     9260           
=========================================
  Hits              ?    15126           
  Misses            ?    36330           
  Partials          ?     1011           

☔ View full report in Codecov by Harness.
📢 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.

@agsaru agsaru marked this pull request as ready for review June 9, 2026 05:26
@agsaru agsaru requested a review from Copilot June 9, 2026 05:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comment thread test/ux/core/test_basic.py
Comment thread test/ux/core/test_resume.py Outdated
Comment thread test/ux/core/test_basic.py
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