test: implement py testing good practices

Author: mdevolde

tests/benchmarks/test_bench_check.py

@@ -1,6 +1,10 @@
 """Benchmark tests for LanguageTool grammar checking performance.
 
 Run with: pytest tests/benchmarks/ -v
+
+A JVM is required to run these benchmarks. If the local LanguageTool JAR cache is
+empty, the first ``LanguageTool(...)`` call in this module triggers a real download
+of the LanguageTool archive over the network.
 """
 
 from __future__ import annotations
@@ -23,18 +27,36 @@
 
 @pytest.fixture(scope="module")
 def tool() -> Generator[language_tool_python.LanguageTool, None, None]:
-    """Provide a LanguageTool instance shared across benchmarks in this module."""
+    """Provide a LanguageTool instance shared across benchmarks in this module.
+
+    Performs one warm-up ``check()`` call before yielding, since pytest.ini does
+    not configure ``--benchmark-warmup``: without it, whichever benchmark happens
+    to run first in this module would absorb the server's cold-start JIT cost.
+    """
     with language_tool_python.LanguageTool("en-US") as t:
+        t.check("warm-up")
         yield t
 
 
 @pytest.fixture(scope="module")
 def cached_tool() -> Generator[language_tool_python.LanguageTool, None, None]:
-    """Provide a pipeline-caching LanguageTool instance for cache benchmarks."""
+    """Provide a pipeline-caching LanguageTool instance for cache benchmarks.
+
+    ``cacheSize=1000`` sets the maximum number of previously checked sentences the
+    server keeps in memory, ``pipelineCaching=True`` additionally caches the
+    internal per-language analysis pipeline (tokenizer, tagger, etc.) so it is not
+    rebuilt on every request. Together they let repeated checks of the same
+    sentence skip most of the analysis work.
+
+    Performs one warm-up ``check()`` call (on text distinct from the benchmarked
+    sentence, so it does not itself pre-populate the cache for that sentence)
+    before yielding, for the same cold-start reason as the ``tool`` fixture above.
+    """
     with language_tool_python.LanguageTool(
         "en-US",
         config={"cacheSize": 1000, "pipelineCaching": True},
     ) as t:
+        t.check("warm-up, unrelated to any benchmarked sentence")
         yield t
 
 
@@ -76,6 +98,10 @@ def test_bench_check_with_pipeline_cache(
 ) -> None:
     """Benchmark grammar checking with pipeline caching enabled.
 
-    Compare with test_bench_check_short_text to measure cache speedup.
+    Every round checks the same ``_SHORT_TEXT`` on the same server instance, so
+    (after the fixture's warm-up call) all rounds but the very first are cache
+    hits. Compare the resulting numbers with ``test_bench_check_short_text``
+    (same text, no caching configured) to estimate the cache's speedup, this
+    test alone does not exercise a cache miss/hit contrast within itself.
     """
     benchmark(cached_tool.check, _SHORT_TEXT)

tests/integration/test_api_public.py

@@ -1,69 +1,65 @@
 """Integration tests for the public API functionality."""
 
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
 import pytest
 
 import language_tool_python
 from language_tool_python.exceptions import RateLimitError
 
+if TYPE_CHECKING:
+    from language_tool_python.match import Match
+
+_ES_TEXT = (
+    "Escriba un texto aquí. LanguageTool le ayudará a afrentar "
+    "algunas dificultades propias de la escritura. Se a hecho un esfuerzo "
+    "para detectar errores tipográficos, ortograficos y incluso "
+    "gramaticales. También algunos errores de estilo, a grosso modo."
+)
 
-def test_remote_es() -> None:
-    """Test the public API with Spanish language text.
 
-    This test verifies that the LanguageToolPublicAPI correctly identifies various
-    errors in a Spanish text sample.
+@pytest.fixture(scope="module")
+def es_matches() -> list[Match]:
+    """Check ``_ES_TEXT`` once against the public API and share the result.
 
-    :raises AssertionError: If the detected matches do not match the expected output.
+    The check is only performed once (module-scoped) to limit the number of
+    requests sent to the public API. If the request is rate-limited, every test
+    depending on this fixture is skipped silently rather than failing.
     """
     try:
         with language_tool_python.LanguageToolPublicAPI("es") as tool:
-            es_text = (
-                "Escriba un texto aquí. LanguageTool le ayudará a afrentar "
-                "algunas dificultades propias de la escritura. Se a hecho un esfuerzo "
-                "para detectar errores tipográficos, ortograficos y incluso "
-                "gramaticales. También algunos errores de estilo, a grosso modo."
-            )
-            matches = tool.check(es_text)
-            assert (
-                str(matches)
-                == """[Match({'rule_id': 'AFRENTAR_DIFICULTADES', 'message': 'Confusión
-                   entre «afrontar» y «afrentar».', 'replacements': ['afrontar'],
-                   'offset_in_context': 43, 'context': '...n texto aquí.
-
-                   LanguageTool le ayudará a afrentar algunas dificultades propias de la
-                   escr...', 'offset': 49, 'error_length': 8, 'category':
-                   'INCORRECT_EXPRESSIONS', 'rule_issue_type': 'grammar', 'sentence':
-                   'LanguageTool le ayudará a afrentar algunas dificultades propias de
-                   la escritura.'}), Match({'rule_id': 'PRON_HABER_PARTICIPIO',
-                   'message': 'El v. ‘haber’ se escribe con hache.',
-                   'replacements': ['ha'], 'offset_in_context': 43, 'context':
-                   '...ificultades propias de la escritura. Se a hecho un esfuerzo para
-                   detectar errores...', 'offset': 107, 'error_length': 1, 'category':
-                   'MISSPELLING', 'rule_issue_type': 'misspelling', 'sentence': 'Se a
-                   hecho un esfuerzo para detectar errores tipográficos, ortograficos y
-                   incluso gramaticales.'}), Match({'rule_id': 'MORFOLOGIK_RULE_ES',
-                   'message': 'Se ha encontrado un posible error ortográfico.',
-                   'replacements': ['ortográficos', 'ortográficas', 'ortográfico',
-                   'orográficos', 'ortografiaos', 'ortografíeos'], 'offset_in_context':
-                   43, 'context': '...rzo para detectar errores tipográficos,
-                   ortograficos y incluso gramaticales. También algunos...', 'offset':
-                   163, 'error_length': 12, 'category': 'TYPOS', 'rule_issue_type':
-                   'misspelling', 'sentence': 'Se a hecho un esfuerzo para detectar
-                   errores tipográficos, ortograficos y incluso gramaticales.'}),
-                   Match({'rule_id': 'Y_E_O_U', 'message': 'Cuando precede a palabras
-                   que comienzan por ‘i’, la conjunción ‘y’ se
-                   transforma en ‘e’.', 'replacements': ['e'],
-                   'offset_in_context': 43, 'context': '...ctar errores tipográficos,
-                   ortograficos y incluso gramaticales. También algunos e...', 'offset':
-                   176, 'error_length': 1, 'category': 'GRAMMAR', 'rule_issue_type':
-                   'grammar', 'sentence': 'Se a hecho un esfuerzo para detectar errores
-                   tipográficos, ortograficos y incluso gramaticales.'}),
-                   Match({'rule_id': 'GROSSO_MODO', 'message': 'Esta expresión latina se
-                   usa sin preposición.', 'replacements': ['grosso modo'],
-                   'offset_in_context': 43, 'context': '...les. También algunos errores
-                   de estilo, a grosso modo.', 'offset': 235, 'error_length': 13,
-                   'category': 'GRAMMAR', 'rule_issue_type': 'grammar', 'sentence':
-                   'También algunos errores de estilo, a grosso modo.'})]
-                   """
-            )
+            return tool.check(_ES_TEXT)
     except RateLimitError:
         pytest.skip("Rate limit exceeded for public API.")
+
+
+@pytest.mark.parametrize(
+    ("rule_id", "category", "offset"),
+    [
+        ("AFRENTAR_DIFICULTADES", "INCORRECT_EXPRESSIONS", 49),
+        ("PRON_HABER_PARTICIPIO", "MISSPELLING", 107),
+        ("MORFOLOGIK_RULE_ES", "TYPOS", 163),
+        ("Y_E_O_U", "GRAMMAR", 176),
+        ("GROSSO_MODO", "GRAMMAR", 235),
+    ],
+)
+def test_remote_es(
+    es_matches: list[Match],
+    rule_id: str,
+    category: str,
+    offset: int,
+) -> None:
+    """Test that the public API detects a specific known error in Spanish text.
+
+    LanguageTool rules can change over time, so this asserts on individual match
+    fields (rule_id, category, offset) rather than requiring the entire response
+    to match a frozen snapshot exactly.
+
+    :raises AssertionError: If no match with the expected fields is found.
+    """
+    assert any(
+        m.rule_id == rule_id and m.category == category and m.offset == offset
+        for m in es_matches
+    )

tests/integration/test_cli.py

@@ -37,9 +37,7 @@ def remote_server() -> Generator[tuple[str, int], None, None]:
     :rtype: Generator[Tuple[str, int], None, None]
     """
     with language_tool_python.LanguageTool("en-US") as tool:
-        host = tool._host
-        port = tool._port
-        yield host, port
+        yield tool.host, tool.port
 
 
 @pytest.mark.parametrize(

tests/integration/test_config.py

@@ -135,29 +135,60 @@ def test_config_caching() -> None:
 
     This test verifies that LanguageTool's caching mechanism (cacheSize and
     pipelineCaching) significantly improves performance when checking the same text
-    multiple times. The test measures the time difference between the first and second
-    checks to ensure caching provides a substantial speedup.
+    multiple times. The test measures the time difference between an uncached and a
+    cached check to ensure caching provides a substantial speedup.
+
+    This is inherently a timing-sensitive test and could still be flaky under heavy
+    machine load, so it: (1) performs a warm-up check on unrelated text before
+    timing, to exclude one-off JIT/connection-setup costs from the measurement
+    without pre-populating the cache for the text under test, and (2) repeats the
+    timed comparison up to ``_ATTEMPTS`` times, succeeding as soon as one attempt
+    shows the expected speedup, instead of requiring every attempt to pass.
 
     :raises AssertionError: If caching does not provide the expected performance
-        improvement.
+        improvement in any attempt.
     """
+    speedup_factor = 5.0
+    attempts = 3
+
     with language_tool_python.LanguageTool(
         "en-US",
         config={"cacheSize": 1000, "pipelineCaching": True},
     ) as tool:
+        tool.check("warm-up text unrelated to the cached sentence below")
+
         s = "hello darkness my old frend"
-        t1 = time.time()
-        tool.check(s)
-        t2 = time.time()
-        tool.check(s)
-        t3 = time.time()
-
-        # This is a silly test that says: caching should speed up a grammar-checking
-        # by a factor of speed_factor when checking the same sentence twice. It
-        # theoretically could be very flaky.
-        # But in practice I've observed speedup of around 250x (6.76s to 0.028s).
-        speedup_factor = 10.0
-        assert (t2 - t1) / speedup_factor > (t3 - t2)
+        for _ in range(attempts):
+            t1 = time.time()
+            tool.check(s)
+            t2 = time.time()
+            tool.check(s)
+            t3 = time.time()
+
+            # In practice, speedups of around 250x (6.76s to 0.028s) have been observed.
+            if (t2 - t1) / speedup_factor > (t3 - t2):
+                return
+
+        pytest.fail(
+            f"Caching did not provide the expected speedup in {attempts} attempts."
+        )
+
+
+def test_inexistent_language() -> None:
+    """Test that creating a LanguageTag with an invalid language code raises an error.
+
+    This test verifies that the LanguageTag constructor correctly validates language
+    codes and raises a ValueError when given a language code that is not supported.
+    A real server is required here to obtain the list of supported languages via
+    ``tool._get_languages()``.
+
+    :raises AssertionError: If ValueError is not raised for an invalid language code.
+    """
+    with (
+        language_tool_python.LanguageTool("en-US") as tool,
+        pytest.raises(ValueError, match="unsupported language"),
+    ):
+        language_tool_python.LanguageTag("xx-XX", tool._get_languages())
 
 
 def test_disabled_rule_in_config() -> None:

tests/integration/test_download.py

@@ -33,21 +33,6 @@ def test_install_too_old_version() -> None:
         language_tool_python.LanguageTool(language_tool_download_version="3.9")
 
 
-def test_inexistent_language() -> None:
-    """Test that creating a LanguageTag with an invalid language code raises an error.
-
-    This test verifies that the LanguageTag constructor correctly validates language
-    codes and raises a ValueError when given a language code that is not supported.
-
-    :raises AssertionError: If ValueError is not raised for an invalid language code.
-    """
-    with (
-        language_tool_python.LanguageTool("en-US") as tool,
-        pytest.raises(ValueError, match="unsupported language"),
-    ):
-        language_tool_python.LanguageTag("xx-XX", tool._get_languages())
-
-
 def test_install_oldest_supported_version() -> None:
     """Test that downloading the oldest supported LanguageTool version works correctly.
 

tests/integration/test_server_local.py

@@ -58,7 +58,6 @@ def test_process_starts_and_stops_on_close() -> None:
     # Make sure process stopped after close() was called.
     time.sleep(0.5)  # Give some time for process to stop after close() call.
     assert proc.poll() is not None, "tool._server should stop running after deletion"
-    # remember --> if poll is None: # p.subprocess is alive
 
 
 def test_local_client_server_connection() -> None:
@@ -72,7 +71,7 @@ def test_local_client_server_connection() -> None:
         server.
     """
     with language_tool_python.LanguageTool("en-US", host="127.0.0.1") as tool1:
-        url = f"http://{tool1._host}:{tool1._port}/"
+        url = f"http://{tool1.host}:{tool1.port}/"
         with language_tool_python.LanguageTool("en-US", remote_server=url) as tool2:
             assert len(tool2.check("helo darknes my old frend"))
 
@@ -104,28 +103,31 @@ def test_session_only_new_spellings() -> None:
     initial_checksum = hashlib.sha256(initial_spelling_file_contents.encode())
 
     new_spellings = ["word1", "word2", "word3"]
-    with language_tool_python.LanguageTool(
-        "en-US",
-        new_spellings=new_spellings,
-        new_spellings_persist=False,
-    ) as tool:
-        tool.enabled_rules_only = True
-        tool.enabled_rules = {"MORFOLOGIK_RULE_EN_US"}
-        matches = tool.check(" ".join(new_spellings))
+    try:
+        with language_tool_python.LanguageTool(
+            "en-US",
+            new_spellings=new_spellings,
+            new_spellings_persist=False,
+        ) as tool:
+            tool.enabled_rules_only = True
+            tool.enabled_rules = {"MORFOLOGIK_RULE_EN_US"}
+            matches = tool.check(" ".join(new_spellings))
+
+        with spelling_file_path.open("r", encoding="utf-8") as spelling_file:
+            subsequent_spelling_file_contents = spelling_file.read()
+        subsequent_checksum = hashlib.sha256(subsequent_spelling_file_contents.encode())
 
-    with spelling_file_path.open("r", encoding="utf-8") as spelling_file:
-        subsequent_spelling_file_contents = spelling_file.read()
-    subsequent_checksum = hashlib.sha256(subsequent_spelling_file_contents.encode())
-
-    if initial_checksum != subsequent_checksum:
+        assert not matches
+        assert initial_checksum.hexdigest() == subsequent_checksum.hexdigest()
+    finally:
+        # Restore unconditionally (not just when a diff was detected) so that a
+        # failed assertion above can never leave the shared spelling file corrupted
+        # for the rest of the test session.
         with spelling_file_path.open(
             "w", encoding="utf-8", newline="\n"
         ) as spelling_file:
             spelling_file.write(initial_spelling_file_contents)
 
-    assert not matches
-    assert initial_checksum.hexdigest() == subsequent_checksum.hexdigest()
-
 
 def test_new_spellins_in_es() -> None:
     """Test that new spellings are recognized in Spanish language.