Skip to content

test: reset intentservice state between tests - #2125

Open
lorenzozanee wants to merge 1 commit into
NVIDIA:mainfrom
lorenzozanee:fix/intentservice-test-state-reset
Open

test: reset intentservice state between tests#2125
lorenzozanee wants to merge 1 commit into
NVIDIA:mainfrom
lorenzozanee:fix/intentservice-test-state-reset

Conversation

@lorenzozanee

Copy link
Copy Markdown

What

intentservice keeps its state in module globals (is_loaded, intent_typology, intent_detectors, intents_active) and nothing resets them between tests. The suite runs in a single process, so whichever test last called load() decides what every later test sees. This is the same problem langservice had, already fixed by clear_langprovider_state in tests/langservice/conftest.py; intentservice still needs the equivalent.

Concrete symptom: test_invalid_intents_rejected never loads the service itself, so it only passes because an earlier test leaves the service loaded — run alone it raises GarakException: get_intent_stubs called on non-loaded intentservice instead of the expected ValueError. Intent-probe translation tests in tests/langservice/probes/test_probes_base.py also flip their skip reason depending on whether an earlier test loaded the service. (The issue's example uses probes.tap.TAPIntent, which no longer exists on main; the same flip reproduces with the remaining IntentProbe, probes.grandma.GrandmaIntent.)

Fix

  • autouse clear_intentservice_state fixture in tests/conftest.py, mirroring clear_langprovider_state: resets the four module globals after every test
  • test_invalid_intents_rejected now loads the service itself instead of relying on leaked state
  • new regression pair in tests/cas/test_intentservice_state_reset.py: the first test loads the service and asserts it is populated, the second asserts the state was reset in between — the second test fails on main without the fixture

I searched the open PRs and none address intentservice test state; the only intentservice-flavoured open PR (#2030) is about harness intent handling.

Verification

python -m pytest tests/cas tests/test_payloads.py tests/harnesses/test_harness_intent_plugin_cache.py -q
337 passed, 1 skipped

python -m pytest tests/cas/test_intentservice_state_reset.py -v
2 passed   (without the fixture the second test fails on main:
            AssertionError: intentservice must not stay loaded across tests)

# order independence, previously inconsistent:
python -m pytest -p no:cacheprovider "tests/cas/test_intentservice.py::test_load_intentservice" "tests/langservice/probes/test_probes_base.py::test_probe_prompt_translation[probes.grandma.GrandmaIntent]" -rs
python -m pytest -p no:cacheprovider "tests/langservice/probes/test_probes_base.py::test_probe_prompt_translation[probes.grandma.GrandmaIntent]" "tests/cas/test_intentservice.py::test_load_intentservice" -rs
both orders now skip with the same reason; before the change the skip reason flipped
  • Verify the thing does what it should
  • Verify the thing does not do what it should not

Closes #2124

intentservice keeps its state in module globals and nothing resets them
between tests, so any test calling load() leaked a loaded service into
every test that ran after it. test_invalid_intents_rejected only passed
because of that leak - on its own it raised GarakException instead of
the expected ValueError.

Add an autouse clear_intentservice_state fixture in tests/conftest.py
mirroring clear_langprovider_state, make test_invalid_intents_rejected
load the service itself, and add a regression pair in
tests/cas/test_intentservice_state_reset.py proving the reset.

Closes NVIDIA#2124

Signed-off-by: lorenzozanee <wyz0707@proton.me>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is no reason to have a test file to test testing only functionality.

At best test_intentservice_load_populates_state asserts are functionality that should be added to test_load_intentservice in test_intentservice.py.

Comment thread tests/conftest.py
reload()


@pytest.fixture(autouse=True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Setting this to autouse at the root of the test path will cause it to run for every test. Is that really what it should do?

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.

intentservice global state (is_loaded/intents_active) not reset between tests

2 participants