omniforge/notebooklm-py-analysis
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix(test): unpack server_conversation_id from chat response tuples (#145)

Browse source 
* fix(test): unpack server_conversation_id from chat response tuples

_parse_ask_response_with_references now returns 3 values (answer, refs,
server_conv_id) and _extract_answer_and_refs_from_chunk returns 4 values
(text, is_answer, refs, conv_id), but 19 test call sites were still
unpacking the old 2/3-value signatures, causing ValueError on all CI
platforms.

* test: assert conv_id is None in chat response unpack tests

Address reviewer feedback: replace _conv_id throwaway with conv_id and
add explicit assertions to verify the returned server_conversation_id is
None in all test cases where no string conversation ID is present in the
response payload.
This commit is contained in:
Teng Lin 2026-03-03 14:36:44 -08:00 committed by GitHub
parent ac58d7f225
commit bc30a3d97b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 56 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

75
tests/integration/test_chat.py
View file

@ -1054,8 +1054,9 @@ class TestParseAskResponseEdgeCases:
chunk_json = json.dumps([["wrb.fr", None, inner_json]])
response_body = f")]}}'\n{len(chunk_json)}\n{chunk_json}\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
assert answer == "Answer text."
assert conv_id is None
def test_parse_response_without_prefix(self, auth_tokens):
"""Test response not starting with )]}' is parsed directly."""
@ -1067,15 +1068,17 @@ class TestParseAskResponseEdgeCases:
chunk_json = json.dumps([["wrb.fr", None, inner_json]])
response_body = f"{len(chunk_json)}\n{chunk_json}\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
assert answer == "Answer without prefix."
assert conv_id is None
def test_parse_empty_response_returns_empty_string(self, auth_tokens):
"""Test that completely empty response returns empty string (line 489)."""
client = NotebookLMClient(auth_tokens)
answer, refs = client.chat._parse_ask_response_with_references(")]}'\n")
answer, refs, conv_id = client.chat._parse_ask_response_with_references(")]}'\n")
assert answer == ""
assert refs == []
assert conv_id is None
def test_parse_response_no_marked_answer_falls_back_to_unmarked(self, auth_tokens):
"""Test fallback to unmarked text when no marked answer exists (line 486)."""
@ -1096,8 +1099,9 @@ class TestParseAskResponseEdgeCases:
chunk_json = json.dumps([["wrb.fr", None, inner_json]])
response_body = f")]}}'\n{len(chunk_json)}\n{chunk_json}\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
assert answer == "This is unmarked text content."
assert conv_id is None
def test_parse_response_assigns_citation_numbers(self, auth_tokens):
"""Test that citation_number is assigned based on order of appearance (line 462-463)."""
@ -1136,9 +1140,10 @@ class TestParseAskResponseEdgeCases:
chunk_json = json.dumps([["wrb.fr", None, inner_json]])
response_body = f")]}}'\n{len(chunk_json)}\n{chunk_json}\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
assert len(refs) == 1
assert refs[0].citation_number == 1
assert conv_id is None
class TestExtractAnswerAndRefsFromChunk:
@ -1147,22 +1152,26 @@ class TestExtractAnswerAndRefsFromChunk:
def test_invalid_json_returns_none(self, auth_tokens):
"""Test that invalid JSON input returns (None, False, []) (line 496-527)."""
client = NotebookLMClient(auth_tokens)
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk("not-valid-json")
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
"not-valid-json"
)
assert text is None
assert is_answer is False
assert refs == []
assert conv_id is None
def test_non_list_data_returns_none(self, auth_tokens):
"""Test that non-list JSON data returns (None, False, []) (line 530)."""
import json
client = NotebookLMClient(auth_tokens)
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps("a string value")
)
assert text is None
assert is_answer is False
assert refs == []
assert conv_id is None
def test_item_not_wrb_fr_is_skipped(self, auth_tokens):
"""Test that items where item[0] != 'wrb.fr' are skipped (line 540)."""
@ -1170,8 +1179,11 @@ class TestExtractAnswerAndRefsFromChunk:
client = NotebookLMClient(auth_tokens)
data = [["other.fr", "method_id", json.dumps([[["answer"]]])]]
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps(data))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps(data)
)
assert text is None
assert conv_id is None
def test_inner_json_not_string_is_skipped(self, auth_tokens):
"""Test that non-string inner_json is skipped (line 544)."""
@ -1180,8 +1192,11 @@ class TestExtractAnswerAndRefsFromChunk:
client = NotebookLMClient(auth_tokens)
# item[2] is an integer, not a string
data = [["wrb.fr", "method_id", 42, None, None]]
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps(data))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps(data)
)
assert text is None
assert conv_id is None
def test_inner_data_first_not_list_is_skipped(self, auth_tokens):
"""Test that inner_data[0] that is not a list is skipped (line 546)."""
@ -1191,8 +1206,11 @@ class TestExtractAnswerAndRefsFromChunk:
# inner_data[0] is a string, not a list
inner_data = ["not a list"]
data = [["wrb.fr", "method_id", json.dumps(inner_data)]]
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps(data))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps(data)
)
assert text is None
assert conv_id is None
def test_inner_data_first_text_not_string_is_skipped(self, auth_tokens):
"""Test that non-string first[0] text is skipped (line 546)."""
@ -1202,8 +1220,11 @@ class TestExtractAnswerAndRefsFromChunk:
# first[0] is None, not a string
inner_data = [[[None, None, [12345]]]]
data = [["wrb.fr", "method_id", json.dumps(inner_data)]]
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps(data))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps(data)
)
assert text is None
assert conv_id is None
def test_inner_json_invalid_json_continues(self, auth_tokens):
"""Test that invalid inner JSON is caught and processing continues (line 560-561)."""
@ -1212,19 +1233,25 @@ class TestExtractAnswerAndRefsFromChunk:
client = NotebookLMClient(auth_tokens)
# item[2] is a string but not valid JSON
data = [["wrb.fr", "method_id", "{not valid json}"]]
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps(data))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps(data)
)
assert text is None
assert refs == []
assert conv_id is None
def test_returns_none_when_no_wrb_fr_items(self, auth_tokens):
"""Test that empty list or no matching items returns (None, False, [])."""
import json
client = NotebookLMClient(auth_tokens)
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps([]))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps([])
)
assert text is None
assert is_answer is False
assert refs == []
assert conv_id is None
def test_item_with_too_few_elements_is_skipped(self, auth_tokens):
"""Test items with len < 3 are skipped."""
@ -1232,8 +1259,11 @@ class TestExtractAnswerAndRefsFromChunk:
client = NotebookLMClient(auth_tokens)
data = [["wrb.fr", "only_two"]]
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps(data))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps(data)
)
assert text is None
assert conv_id is None
class TestParseCitationsEdgeCases:
@ -1556,9 +1586,10 @@ class TestParseAskResponseNumericLengthPrefix:
# Append a dangling numeric line at the end with no content following
response_body = f")]}}'\n{len(chunk_json)}\n{chunk_json}\n99\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
# The valid chunk should still be parsed
assert answer == "Valid answer."
assert conv_id is None
def test_parse_response_with_direct_json_line_no_prefix(self, auth_tokens):
"""Test response where JSON lines are direct (not length-prefixed) (lines 471-473)."""
@ -1572,8 +1603,9 @@ class TestParseAskResponseNumericLengthPrefix:
# No length prefix - just direct JSON
response_body = f")]}}'\n{chunk_json}\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
assert answer == "Direct JSON answer."
assert conv_id is None
class TestExtractAnswerEmptyInnerData:
@ -1587,9 +1619,12 @@ class TestExtractAnswerEmptyInnerData:
# inner_data is an empty list -> len(inner_data) == 0, skips the processing
inner_data: list = []
data = [["wrb.fr", "method_id", json.dumps(inner_data)]]
text, is_answer, refs = client.chat._extract_answer_and_refs_from_chunk(json.dumps(data))
text, is_answer, refs, conv_id = client.chat._extract_answer_and_refs_from_chunk(
json.dumps(data)
)
assert text is None
assert is_answer is False
assert conv_id is None
class TestExtractTextPassagesMultiplePassages:
@ -1690,9 +1725,10 @@ class TestParseAskResponseBranchCoverage:
# Both chunks with marked answers
response_body = f")]}}'\n{make_chunk(longer_json)}\n{make_chunk(shorter_json)}\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
# Longer marked answer wins
assert answer == "This is the longer marked answer text."
assert conv_id is None
def test_citation_number_not_reassigned_when_already_set(self, auth_tokens):
"""Test that citation_number already set is not overwritten (arc 496->495)."""
@ -1747,9 +1783,10 @@ class TestParseAskResponseBranchCoverage:
# this arc may not be reachable in normal flow - it's a defensive check.
# Let's verify the assignment logic works correctly with multiple refs.
response_body = f")]}}'\n{len(chunk_json)}\n{chunk_json}\n"
answer, refs = client.chat._parse_ask_response_with_references(response_body)
answer, refs, conv_id = client.chat._parse_ask_response_with_references(response_body)
assert len(refs) == 1
assert refs[0].citation_number == 1
assert conv_id is None
class TestExtractTextPassagesNonIntEndChar: