Standardize test function names for clarity and consistency#7709
Standardize test function names for clarity and consistency#7709eddyashton wants to merge 4 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes naming for selected Python end-to-end test functions to better distinguish top-level runners/groups from individual test cases and helpers, and adds documentation describing the intended test hierarchy and naming conventions.
Changes:
- Renames several
test_*functions torun_*or non-test_helper names and updates their call sites. - Renames a shared “main tests” helper in
e2e_logging.pyto avoid therun_prefix. - Adds
tests/README.mddocumenting the test hierarchy and naming conventions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/recovery.py | Renames a function used for recovery-from-files flow and updates its invocation. |
| tests/npm_tests.py | Renames an internal helper from test_ to a non-test helper name and updates usage. |
| tests/e2e_operations.py | Renames standalone checks to run_* and updates entry-point calls; introduces new naming alignment considerations. |
| tests/e2e_logging.py | Renames a shared helper to avoid implying it creates/owns a network. |
| tests/README.md | Documents intended test hierarchy and naming conventions. |
|
|
||
| def test_error_message_on_failure_to_fetch_snapshot(const_args): | ||
| def run_error_message_on_failure_to_fetch_snapshot(const_args): | ||
| args = copy.deepcopy(const_args) |
There was a problem hiding this comment.
This function creates a new network from a copy.deepcopy(const_args) but does not update args.label. Elsewhere in this file, when deep-copying args for a separate network, a descriptive label suffix is appended to avoid workspace directory collisions/overwrites (e.g. run_manual_snapshot_tests). Add a suffix to args.label here too.
| args = copy.deepcopy(const_args) | |
| args = copy.deepcopy(const_args) | |
| args_label = getattr(args, "label", None) | |
| if args_label: | |
| args.label = f"{args_label}.snapshot_fetch_failure" | |
| else: | |
| args.label = "snapshot_fetch_failure" |
| network.start_and_open(args) | ||
|
|
||
| run_main_tests(network, args) | ||
| do_main_tests(network, args) |
There was a problem hiding this comment.
Shall we do it for all tests then? Or are these the only non-top-level functions that are named run_...?
There was a problem hiding this comment.
This is not a top-level, this is inside a run_* already. Usually it's:
def run_foo(args):
with network():
test_bar(network, args)
test_baz(network, args)
In this instance, we had a big list of test_* functions that we want to call from 2 different top-level run_* functions (with different network setups/prior state, I forget the actual matrix). It's (apparently) the only case we have that pattern, so we give it a different name.
No description provided.