test(api): delete protocol_api_old/test_context.py? by ddcc4 · Pull Request #18412 · Opentrons/opentrons

ddcc4 · 2025-05-21T21:38:48Z

Overview

Is this a good idea??!?

protocol_api_old/test_context.py is an old test that monkeypatches aspirate() and dispense() in a very particular way:

def fake_aspirate(
    vol: Optional[float] = None,
    loc: Optional[Location] = None,
    rate: Optional[int] = None,
)

These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call aspirate() and dispense() with positional arguments (not named arguments), and instrument_context.py has to avoid calling aspirate()/dispense() with any of our new arguments that are not available in the fake (like push_out or flow_rate).

I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it?

Review requests

I'd like feedback from anyone who knows what protocol_api_old is, and what the purpose of protocol_api_old/test_context.py is.

Risk assessment

A test-only change.

sfoster1

go for it imo. i think it is because we wanted to refresh the tests but keep the old ones just in case, IMO it's no longer useful

SyntaxColoring

Agreed. Nuke it.

Thank you!

@jbleon95

# Overview Is this a good idea??!? `protocol_api_old/test_context.py` is an old test that monkeypatches `aspirate()` and `dispense()` in a very particular way: ``` def fake_aspirate( vol: Optional[float] = None, loc: Optional[Location] = None, rate: Optional[int] = None, ) ``` These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call `aspirate()` and `dispense()` with positional arguments (not named arguments), and `instrument_context.py` has to avoid calling `aspirate()`/`dispense()` with any of our new arguments that are not available in the fake (like `push_out` or `flow_rate`). I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it? ## Review requests I'd like feedback from anyone who knows what `protocol_api_old` is, and what the purpose of `protocol_api_old/test_context.py` is. ## Risk assessment A test-only change. (cherry picked from commit 9d7aa9f)

@jbleon95

# Overview Is this a good idea??!? `protocol_api_old/test_context.py` is an old test that monkeypatches `aspirate()` and `dispense()` in a very particular way: ``` def fake_aspirate( vol: Optional[float] = None, loc: Optional[Location] = None, rate: Optional[int] = None, ) ``` These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call `aspirate()` and `dispense()` with positional arguments (not named arguments), and `instrument_context.py` has to avoid calling `aspirate()`/`dispense()` with any of our new arguments that are not available in the fake (like `push_out` or `flow_rate`). I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it? ## Review requests I'd like feedback from anyone who knows what `protocol_api_old` is, and what the purpose of `protocol_api_old/test_context.py` is. ## Risk assessment A test-only change. (cherry picked from commit 9d7aa9f)

@jbleon95

# Overview Is this a good idea??!? `protocol_api_old/test_context.py` is an old test that monkeypatches `aspirate()` and `dispense()` in a very particular way: ``` def fake_aspirate( vol: Optional[float] = None, loc: Optional[Location] = None, rate: Optional[int] = None, ) ``` These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call `aspirate()` and `dispense()` with positional arguments (not named arguments), and `instrument_context.py` has to avoid calling `aspirate()`/`dispense()` with any of our new arguments that are not available in the fake (like `push_out` or `flow_rate`). I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it? ## Review requests I'd like feedback from anyone who knows what `protocol_api_old` is, and what the purpose of `protocol_api_old/test_context.py` is. ## Risk assessment A test-only change. (cherry picked from commit 9d7aa9f)

@jbleon95

# Overview Is this a good idea??!? `protocol_api_old/test_context.py` is an old test that monkeypatches `aspirate()` and `dispense()` in a very particular way: ``` def fake_aspirate( vol: Optional[float] = None, loc: Optional[Location] = None, rate: Optional[int] = None, ) ``` These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call `aspirate()` and `dispense()` with positional arguments (not named arguments), and `instrument_context.py` has to avoid calling `aspirate()`/`dispense()` with any of our new arguments that are not available in the fake (like `push_out` or `flow_rate`). I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it? ## Review requests I'd like feedback from anyone who knows what `protocol_api_old` is, and what the purpose of `protocol_api_old/test_context.py` is. ## Risk assessment A test-only change. (cherry picked from commit 9d7aa9f)

@jbleon95

# Overview Is this a good idea??!? `protocol_api_old/test_context.py` is an old test that monkeypatches `aspirate()` and `dispense()` in a very particular way: ``` def fake_aspirate( vol: Optional[float] = None, loc: Optional[Location] = None, rate: Optional[int] = None, ) ``` These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call `aspirate()` and `dispense()` with positional arguments (not named arguments), and `instrument_context.py` has to avoid calling `aspirate()`/`dispense()` with any of our new arguments that are not available in the fake (like `push_out` or `flow_rate`). I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it? ## Review requests I'd like feedback from anyone who knows what `protocol_api_old` is, and what the purpose of `protocol_api_old/test_context.py` is. ## Risk assessment A test-only change. (cherry picked from commit 9d7aa9f)

@jbleon95

# Overview Is this a good idea??!? `protocol_api_old/test_context.py` is an old test that monkeypatches `aspirate()` and `dispense()` in a very particular way: ``` def fake_aspirate( vol: Optional[float] = None, loc: Optional[Location] = None, rate: Optional[int] = None, ) ``` These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call `aspirate()` and `dispense()` with positional arguments (not named arguments), and `instrument_context.py` has to avoid calling `aspirate()`/`dispense()` with any of our new arguments that are not available in the fake (like `push_out` or `flow_rate`). I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it? ## Review requests I'd like feedback from anyone who knows what `protocol_api_old` is, and what the purpose of `protocol_api_old/test_context.py` is. ## Risk assessment A test-only change. (cherry picked from commit 9d7aa9f)

@jbleon95

# Overview Is this a good idea??!? `protocol_api_old/test_context.py` is an old test that monkeypatches `aspirate()` and `dispense()` in a very particular way: ``` def fake_aspirate( vol: Optional[float] = None, loc: Optional[Location] = None, rate: Optional[int] = None, ) ``` These argument names, default values, and number of arguments do not match the current API at all. That means that to make the test pass, we have to call `aspirate()` and `dispense()` with positional arguments (not named arguments), and `instrument_context.py` has to avoid calling `aspirate()`/`dispense()` with any of our new arguments that are not available in the fake (like `push_out` or `flow_rate`). I thought we couldn't change the test because it's enforcing some ancient API behavior that we have to support. But @jbleon95 and @SyntaxColoring think the whole test might be redundant now, so maybe we can just delete it? ## Review requests I'd like feedback from anyone who knows what `protocol_api_old` is, and what the purpose of `protocol_api_old/test_context.py` is. ## Risk assessment A test-only change. (cherry picked from commit 9d7aa9f)

Delete protocol_api_old/test_context.py?

8d1d9c1

ddcc4 requested review from SyntaxColoring and jbleon95 May 21, 2025 21:38

ddcc4 requested a review from a team as a code owner May 21, 2025 21:38

sfoster1 approved these changes May 22, 2025

View reviewed changes

SyntaxColoring approved these changes May 22, 2025

View reviewed changes

ddcc4 merged commit 9d7aa9f into edge May 22, 2025
30 checks passed

ddcc4 deleted the dc-del-old-test_context branch May 22, 2025 18:18

ddcc4 mentioned this pull request May 29, 2025

Candidate chore_release-8.5.0 cherry-pick branch #18379

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(api): delete protocol_api_old/test_context.py?#18412

test(api): delete protocol_api_old/test_context.py?#18412
ddcc4 merged 1 commit into
edgefrom
dc-del-old-test_context

ddcc4 commented May 21, 2025 •

edited

Loading

Uh oh!

sfoster1 left a comment

Uh oh!

SyntaxColoring left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ddcc4 commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Review requests

Risk assessment

Uh oh!

sfoster1 left a comment

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ddcc4 commented May 21, 2025 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading