Skip to content

test(pyamber): add unit tests for update_executor logic#4719

Merged
Yicong-Huang merged 4 commits into
apache:mainfrom
Yicong-Huang:test-pyamber-update-executor
May 3, 2026
Merged

test(pyamber): add unit tests for update_executor logic#4719
Yicong-Huang merged 4 commits into
apache:mainfrom
Yicong-Huang:test-pyamber-update-executor

Conversation

@Yicong-Huang
Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Adds pytest coverage for ExecutorManager.update_executor (extending the existing test_executor_manager.py) and the UpdateExecutorHandler that wraps it (new test_update_executor_handler.py).

Any related issues, documentation, discussions?

Closes #4718.

Test-isolation note (not pinned by these tests): the existing executor_manager fixture has a buggy cleanup guard — if hasattr(manager, "_fs"): manager.close() checks for _fs but the cached_property actually stores under fs, so close() is never invoked. Combined with the fact that every fresh ExecutorManager resets executor_version to 0 and thus reuses the module name udf-v1, this leaves stale modules in sys.modules with paths that point at deleted tmp dirs from earlier tests. The new TestUpdateExecutor therefore avoids asserting on the class of the loaded executor and instead checks dict-state preservation via setattr/getattr, so it works regardless of which cached udf-v1 happens to satisfy the import. A separate fix would untangle the cleanup guard and probably move version numbering off of the per-manager counter.

How was this PR tested?

cd amber/src/main/python
ruff check core/architecture/managers/test_executor_manager.py core/architecture/handlers/control/test_update_executor_handler.py
ruff format --check core/architecture/managers/test_executor_manager.py core/architecture/handlers/control/test_update_executor_handler.py
python -m pytest core/architecture/managers/test_executor_manager.py core/architecture/handlers/control/test_update_executor_handler.py

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (claude-opus-4-7)

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 42.49%. Comparing base (ecc2fab) to head (22a6a0e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4719      +/-   ##
============================================
- Coverage     42.95%   42.49%   -0.46%     
+ Complexity     2035     2032       -3     
============================================
  Files           957      957              
  Lines         34077    34094      +17     
  Branches       3753     3753              
============================================
- Hits          14637    14488     -149     
- Misses        18663    18832     +169     
+ Partials        777      774       -3     
Flag Coverage Δ
amber 40.98% <ø> (ø)
python 84.38% <ø> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds pytest coverage for PyAmber executor hot-swap behavior by testing ExecutorManager.update_executor directly and the UpdateExecutorHandler wrapper to ensure correct delegation and state preservation semantics.

Changes:

  • Extend test_executor_manager.py with a new TestUpdateExecutor suite covering state preservation, version/module increments, and error paths.
  • Add test_update_executor_handler.py to validate handler delegation, return type, and oneof extraction behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
amber/src/main/python/core/architecture/managers/test_executor_manager.py Adds unit tests for ExecutorManager.update_executor, including state carryover and assertion/error cases.
amber/src/main/python/core/architecture/handlers/control/test_update_executor_handler.py Adds unit tests ensuring UpdateExecutorHandler.update_executor delegates correctly and extracts code via get_one_of.

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

Comment thread amber/src/main/python/core/architecture/managers/test_executor_manager.py Outdated
Closes apache#4718

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Yicong-Huang Yicong-Huang force-pushed the test-pyamber-update-executor branch from 1cb6443 to 9926f45 Compare May 3, 2026 01:00
Replace `__dict__.get(key) == value` with an explicit `key in __dict__`
membership check before the value comparison so a key that disappeared
from the executor with an expected value of `None` no longer slips past
on `get()`'s default return.
@Yicong-Huang Yicong-Huang merged commit e99577b into apache:main May 3, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit tests for pyamber update_executor logic

4 participants