-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: deduplicate configured model badges #6275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| """Regression coverage for duplicate configured model entries in the picker.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import shutil | ||
| import subprocess | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
| REPO_ROOT = Path(__file__).parent.parent.resolve() | ||
| UI_JS_PATH = REPO_ROOT / "static" / "ui.js" | ||
| NODE = shutil.which("node") | ||
|
|
||
| pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") | ||
|
|
||
|
|
||
| _DRIVER = r""" | ||
| const fs = require('fs'); | ||
| const ui = fs.readFileSync(process.argv[2], 'utf8'); | ||
| function extractFunc(name) { | ||
| const re = new RegExp('function\\s+' + name + '\\s*\\('); | ||
| const start = ui.search(re); | ||
| if (start < 0) throw new Error(name + ' not found'); | ||
| let i = ui.indexOf('{', start); let depth = 1; i++; | ||
| while (depth > 0 && i < ui.length) { | ||
| if (ui[i] === '{') depth++; | ||
| else if (ui[i] === '}') depth--; | ||
| i++; | ||
| } | ||
| return ui.slice(start, i); | ||
| } | ||
| eval(extractFunc('_normalizeConfiguredModelKey')); | ||
| eval(extractFunc('_isEquivalentConfiguredModelEntry')); | ||
| const cases = JSON.parse(process.argv[3]); | ||
| const result = cases.map(c => _isEquivalentConfiguredModelEntry(c.modelId, c.badge, c.entries)); | ||
| process.stdout.write(JSON.stringify(result)); | ||
| """ | ||
|
|
||
|
|
||
| def _equivalent_cases(tmp_path, cases): | ||
| driver = tmp_path / "driver.js" | ||
| driver.write_text(_DRIVER, encoding="utf-8") | ||
| assert NODE is not None | ||
| result = subprocess.run( | ||
| [NODE, str(driver), str(UI_JS_PATH), json.dumps(cases)], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
| assert result.returncode == 0, result.stderr | ||
| return json.loads(result.stdout) | ||
|
|
||
|
|
||
| def test_picker_rows_preserve_provider_id_for_equivalence_check(): | ||
| """The synthesis loop must compare badge routes against real row providers.""" | ||
| ui = UI_JS_PATH.read_text(encoding="utf-8") | ||
|
|
||
| assert "const providerId=child.dataset&&child.dataset.provider?child.dataset.provider:'';" in ui | ||
| assert "providerId,modelsEndpointError,badge:_getConfiguredModelBadge" in ui | ||
| assert "providerId,\n modelsEndpointError," in ui | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| assert "if(_isEquivalentConfiguredModelEntry(modelId,badge,_modelData)) continue;" in ui | ||
| assert "_existingConfiguredKeys" not in ui | ||
|
|
||
|
|
||
| def test_named_custom_provider_routing_id_does_not_duplicate_picker_row(tmp_path): | ||
| entries = [{"value": "model-a", "providerId": "custom:example"}] | ||
| results = _equivalent_cases( | ||
| tmp_path, | ||
| [ | ||
| { | ||
| "modelId": "@custom:example:model-a", | ||
| "badge": {"provider": "custom:example"}, | ||
| "entries": entries, | ||
| }, | ||
| { | ||
| "modelId": "model-a", | ||
| "badge": {"provider": "custom:example"}, | ||
| "entries": entries, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
| assert results == [True, True] | ||
|
|
||
|
|
||
| def test_same_model_id_from_another_provider_remains_distinct(tmp_path): | ||
| entries = [{"value": "model-a", "providerId": "custom:primary"}] | ||
| results = _equivalent_cases( | ||
| tmp_path, | ||
| [ | ||
| { | ||
| "modelId": "@custom:backup:model-a", | ||
| "badge": {"provider": "custom:backup"}, | ||
| "entries": entries, | ||
| }, | ||
| { | ||
| "modelId": "model-a", | ||
| "badge": {"provider": "custom:backup"}, | ||
| "entries": entries, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
| assert results == [False, False] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dataset.providerassignments always overwrittenThe two
opt.dataset.providerwrites on these lines are unconditionally overwritten four lines later byif(provider) opt.dataset.provider=provider;.provideris computed asrequestedProvider||(badge&&badge.provider)||(rawBadge&&rawBadge.provider)||..., so it can never be falsy while eitherbadge.providerorrawBadge.provideris truthy — the exact conditions that guard lines 3276–3277. Readers may expect the intermediate assignments to survive in some case, but they never do.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!