AI Controls (kill switch) test suite#1508
Conversation
Review: AI Controls (kill switch) test suiteOverall the enterprise-policy machinery, POM additions, and test structure are solid. A few issues worth addressing: Redundant navigation on every testThe
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| """, | ||
| select_elem, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Filtering by label != "Block" is fragile: any third button (or a localised "Block" label) will silently select the wrong element. A dedicated component key in the JSON manifest, or a stable attribute like data-l10n-id / element ID, would be more robust.
b1bf217 to
8ccd654
Compare
|
Addressed the automated review (pushed as an amend):
Deferred, with reasoning:
Re-verified on Firefox Nightly 154: 16 pass / 9 skip / 0 fail. |
Review: AI Controls Kill Switch Test SuiteGood overall — clear policy-injection mechanism, well-documented methods, and appropriate use of Double navigation in every test
Silent C3279955 doesn't exercise the "policy removed" scenario |
| ) | ||
| if not matched: | ||
| raise ValueError(f"Provider '{provider}' not found in chatbot dropdown") | ||
| return self |
There was a problem hiding this comment.
Missing post-set confirmation. set_ai_translations calls self.expect(lambda _: self.get_ai_translations_state() == state) after the JS dispatch; this method should do the same:
| return self | |
| if not matched: | |
| raise ValueError(f"Provider '{provider}' not found in chatbot dropdown") | |
| self.expect(lambda _: self.get_ai_chatbot_provider() == provider) | |
| return self |
Without it, C3341325 can pass before the moz-select's async update has propagated the new value to the underlying pref.
| try: | ||
| policy_file.unlink() | ||
| logging.info(f"Removed enterprise policies file {policy_file}") | ||
| except OSError: |
There was a problem hiding this comment.
Silent failure here could leave a policies.json on disk and poison the next test run. At minimum log the error:
| except OSError: | |
| except OSError as exc: | |
| logging.warning(f"Failed to remove enterprise policies file {policy_file}: {exc}") |
| @pytest.fixture() | ||
| def policies_list(): | ||
| """No policy active — simulates policy being removed/changed.""" | ||
| return {} |
There was a problem hiding this comment.
This policies_list = {} means no policy is ever written, so the test only asserts the default (policy-free) state. To validate "enable AI features when policy is changed/removed", you'd need to start with a blocking policy active (i.e. return a non-empty dict here) and then confirm the UI reflects no policy once it's removed. As written the test is a no-op for its stated purpose — which is fine since it's disabled in the manifest, but worth noting if it's ever re-enabled.
8ccd654 to
23e1351
Compare
| policy_file.write_text(json.dumps({"policies": policies_list}, indent=2)) | ||
| wrote_policy = True | ||
| logging.info(f"Wrote enterprise policies to {policy_file}") | ||
| except PermissionError: |
There was a problem hiding this comment.
The write catches only PermissionError, but the unlink above catches the broader OSError. On some systems (e.g. read-only mount, EROFS) a non-permission OSError is raised for writes too. For consistency and correctness, catch OSError here as well — or at minimum re-raise unexpected OSError subtypes.
| except PermissionError: | |
| except OSError: | |
| pytest.skip(f"Cannot write policies to {dist_dir} (permission denied).") |
| if policies_list: | ||
| try: | ||
| dist_dir.mkdir(parents=True, exist_ok=True) | ||
| policy_file.write_text(json.dumps({"policies": policies_list}, indent=2)) |
There was a problem hiding this comment.
Path.write_text uses the platform default encoding when no encoding is specified, which can differ on Windows (e.g. cp1252). JSON files should always be UTF-8.
| policy_file.write_text(json.dumps({"policies": policies_list}, indent=2)) | |
| policy_file.write_text(json.dumps({"policies": policies_list}, indent=2), encoding="utf-8") |
| # TODO: Implement the following to complete this test: | ||
| # 1. Navigate to about:inference (or use IndexedDB / profile inspection) | ||
| # to list cached AI models before and after blocking. | ||
| # 2. Assert that all model entries are removed once the killswitch is on. |
There was a problem hiding this comment.
This test is disabled in the manifest today, but as written it would pass trivially if ever enabled — the TODO steps are the entire substance of the case. Consider adding pytest.skip("Not implemented: …") at the top of the function so enabling it in the manifest can't accidentally produce a false-green.
|
Good overall structure — the enterprise-policy fixture is a clean addition and the reasoning for each disabled/skip case is well-documented. A few things worth addressing: Minor bugs
Double navigation
C3343878 false-green risk
|
23e1351 to
881e74d
Compare
|
Good overall structure and well-reasoned use of 1.
2. Double navigation in tests
3. except PermissionError:
pytest.skip(...)Writing the policy file can fail with other 4. The existing |
| """ | ||
| if block != self.get_ai_killswitch_state(): | ||
| self.driver.execute_script( | ||
| "Services.prefs.setStringPref('browser.ai.control.default', arguments[0]);", |
There was a problem hiding this comment.
Services.prefs is a privileged API. Every other call in the codebase that uses it is decorated with @BasePage.context_chrome (see browser_object_sidebar.py). set_ai_blocking and get_ai_killswitch_state should follow suit, otherwise they rely on about:preferences happening to run privileged — which is correct today but fragile.
| "Services.prefs.setStringPref('browser.ai.control.default', arguments[0]);", | |
| if block != self.get_ai_killswitch_state(): | |
| with self.driver.context(self.driver.CONTEXT_CHROME): | |
| self.driver.execute_script( | |
| "Services.prefs.setStringPref('browser.ai.control.default', arguments[0]);", | |
| "blocked" if block else "available", | |
| ) | |
| self.expect(lambda _: self.get_ai_killswitch_state() == block) |
| self.click_on("ai-controls-toggle") | ||
| self.element_visible("ai-controls-disable-dialog-button") | ||
| buttons = self.get_elements("ai-controls-disable-dialog-button") | ||
| cancel = [el for el in buttons if el.get_attribute("label") == "Cancel"] |
There was a problem hiding this comment.
Bare assert can be silently disabled with Python's -O flag, and it's inconsistent with toggle_ai_killswitch_click which lets IndexError propagate on [0]. Prefer an explicit guard:
| cancel = [el for el in buttons if el.get_attribute("label") == "Cancel"] | |
| cancel = [el for el in buttons if el.get_attribute("label") == "Cancel"] | |
| if not cancel: | |
| raise ValueError("Cancel button not found in block-AI confirmation dialog") | |
| cancel[0].click() |
jsegunosidefox
added
the
skip-claude-review
Summary (revised)Per Ben's feedback, this PR now contains only tests that actually run and assert real behavior — no stubs, no Result: Tests — all pass on Firefox Nightly, en-US (8 collected / 8 passed / 0 skipped)
Notes for review
(Superseded my earlier summary above, which described the pre-trim version.) |
ben-c-at-moz
left a comment
ben-c-at-moz
left a comment
There was a problem hiding this comment.
I see you haven't requested reviewers, which may mean you're not ready with this PR, but I have some comments you should take a look at before you get to that point.
There was a problem hiding this comment.
Just a heads up, we don't need stubs for tests we've determined were unsuitable for automation, so it would be best to remove this.
There was a problem hiding this comment.
The linked test indicates that each element should be tested with the keyboard, which we can do, but is not present in this test, just FYI
| options.binary_location = fx_executable | ||
| # options.set_preference("app.update.disabledForTesting", False) | ||
|
|
||
| # Enterprise policies live in distribution/policies.json next to the binary. |
There was a problem hiding this comment.
Interesting and potentially very useful. Have we confirmed that automated sessions respect the policy files if they have been written like this?
881e74d to
21faf3f
Compare
Ports the automatable AI Controls ("kill switch") tests onto main and
adds the AboutPrefs helpers they need. Per review, the suite contains
ONLY tests that actually run and assert real behavior — no stubs,
skips, or environment-gated cases.
Tests (all pass on Nightly, en-US):
- c3276002: core AI Controls elements are present/visible
- c3298824: Block-all persists after enabling an individual feature
- c3310314: enabled features remain after unblocking via the toggle
- c3340562: cancel the Block-all confirmation leaves AI enabled
- c3341331: WebExtensions ML API is disabled while AI is blocked
Removed the stub/skipped/unautomatable cases (high-contrast, screen
reader, localization, non-en builds, enterprise policies, sidebar sync,
model deletion) and their now-dead harness/selectors, so conftest.py and
about_prefs.components.json are back to matching main.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
21faf3f to
d68f354
Compare
2 participants
Relevant Links
Bugzilla: n/a
TestRail: S71443 (GenAI Settings / AI Controls)
Description of Code / Doc Changes
Ports the AI Controls ("kill switch") tests onto
main(the follow-up to the merged minimal PoC #1302; supersedes the stale #1140). 22 test cases undertests/ai_controls/, one file per TestRail case.conftest.py— enterprise-policy support: apolicies_listfixture writesdistribution/policies.jsonnext to the binary before launch and removes it in teardown (skips gracefully with a clear message if the dir isn't writable, e.g. a SIP-protected app bundle).modules/page_object_prefs.py—AboutPrefsAI helpers:navigate_to_ai_controls,verify_ai_controls_core_elements_visible,set_ai_blocking,get_ai_killswitch_state, translations/chatbot getters+setters,cancel_ai_killswitch_click,get_extensions_ml_enabled. State is read viaaria-pressed(the innerinputhas no.pressedproperty).modules/data/about_prefs.components.json— addedai-control-link-preview-select,ai-control-smart-tab-groups-select,ai-control-pdf-alt-text-select.manifests/key.yaml— registeredai_controlscases.extensions.ml.enabledflips on block (via the UI toggle — a raw pref set doesn't run the killswitch side effects); C3340562 exercises the real "Block all AI enhancements?" Cancel path.Cases that map to inherently manual/gated scenarios (screen readers, non-English/region builds,
about:inferencemodel storage, high-contrast legibility, telemetry pings, enterprise policy) are leftdisabled/unstablerather than given false-confidence assertions.Process Changes Required
pipenv install)./devsetup.sh)modules/page_object_prefs.py)Screenshots or Explanations
Verified locally on Firefox Nightly 154: 18 pass / 7 skip / 0 fail across
tests/ai_controls. Skips are the 5 policy tests (SIP-protected build dir; run in CI) and 2 non-English@pytest.mark.skipcases.Workflow Checklist
🤖 Generated with Claude Code