Merge pull request #125 from openSUSE/toms/validate_rng

Refactor `validate_rng` to return `CompletedProcess`

Author: tomschr

.github/agents/docbuild.agent.md

@@ -33,8 +33,8 @@ Docbuild builds documentation from DocBook 5/ASCIIDoc, manages XML configs, clon
 
 Run tests:
 
-* Complete tests with `upytest` from the alias script.
-* Single tests with `upytest tests/path/to/test_file.py::test_function_name`
+* Run the complete test suite with `uv run --frozen pytest`.
+* Single tests with `uv run --frozen pytest tests/path/to/test_file.py::test_function_name`.
 
 
 ## Constraints & Safety

changelog.d/125.refactor.rst

@@ -0,0 +1,2 @@
+In function :fun:`validate_rng`, use ``CompletedProcess``  as return object.
+Replace tuple with CompletedProcess. This is used in other functions too and this change makes it consistent with other.

src/docbuild/cli/cmd_validate/process.py

@@ -9,6 +9,7 @@
 
 from lxml import etree
 from rich.console import Console
+from subprocess import CompletedProcess
 
 from ...config.xml.checks import CheckResult, register_check
 from ...config.xml.stitch import create_stitchfile
@@ -83,7 +84,7 @@ async def validate_rng(
     *,
     xinclude: bool = True,
     idcheck: bool = True
-) -> tuple[bool, str]:
+) -> CompletedProcess:
     """Validate an XML file against a RELAX NG schema using jing.
 
     If `xinclude` is True (the default), this function resolves XIncludes by
@@ -107,8 +108,6 @@ async def validate_rng(
     try:
         if xinclude:
             # Use a temporary file to store the output of xmllint.
-            # This is more robust than piping, especially if jing doesn't
-            # correctly handle stdin (the command "jing schema.rng -" does NOT work.)
             with tempfile.NamedTemporaryFile(
                 prefix='jing-validation',
                 suffix='.xml',
@@ -119,33 +118,33 @@ async def validate_rng(
                 tmp_filepath = Path(tmp_file.name)
 
                 # 1. Run xmllint to resolve XIncludes and save to temp file
-                process = await run_command(
-                    ['xmllint', '--xinclude', '--output',
-                     str(tmp_filepath), str(xmlfile)]
+                xmllint_proc = await run_command(
+                    ['xmllint', '--xinclude', '--output', str(tmp_filepath), str(xmlfile)]
                 )
-                if process.returncode != 0:
-                    return False, f'xmllint failed: {process.stderr}'
+                if xmllint_proc.returncode != 0:
+                    # Construct a CompletedProcess representing failure from xmllint
+                    return CompletedProcess(
+                        args=['xmllint', str(xmlfile)],
+                        returncode=xmllint_proc.returncode,
+                        stdout=xmllint_proc.stdout,
+                        stderr=f'xmllint failed: {xmllint_proc.stderr}',
+                    )
 
                 # 2. Run jing on the resolved temporary file
                 jing_cmd.append(str(tmp_filepath))
-                process = await run_command(jing_cmd)
-                if process.returncode != 0:
-                    return False, (process.stdout + process.stderr).strip()
-
-                return True, ''
+                return await run_command(jing_cmd)
         else:
             # Validate directly with jing, no XInclude resolution.
             jing_cmd.append(str(xmlfile))
-            process = await run_command(jing_cmd)
-            if process.returncode == 0:
-                return True, ''
-            return False, (process.stdout + process.stderr).strip()
+            return await run_command(jing_cmd)
 
     except FileNotFoundError as e:
         tool = e.filename or 'xmllint/jing'
-        return (
-            False,
-            f'{tool} command not found. Please install it to run validation.',
+        return CompletedProcess(
+            args=[tool],
+            returncode=1,
+            stdout='',
+            stderr=f'{tool} command not found. Please install it to run validation.',
         )
 
 
@@ -203,6 +202,7 @@ async def run_python_checks(
         try:
             result = await asyncio.to_thread(check, tree)
             check_results.append((check.__name__, result))
+
         except Exception as e:
             error_result = CheckResult(success=False, messages=[f'error: {e}'])
             check_results.append((check.__name__, error_result))
@@ -229,7 +229,7 @@ async def process_file(
     )
 
     # IDEA: Should we replace jing and validate with etree.RelaxNG?
-
+    #
     # 1. RNG Validation
     validation_method = context.validation_method
 
@@ -242,22 +242,34 @@ async def process_file(
         )
     elif validation_method == 'jing':
         # Use existing jing-based validator (.rnc or .rng)
-        rng_success, rng_output = await validate_rng(path_obj, idcheck=True)
+        jing_result = await validate_rng(path_obj, idcheck=True)
     else:
         console_err.print(
             f'{shortname:<{max_len}}: RNG validation => [red]failed[/red]'
         )
         console_err.print(f'  [bold red]Error:[/] Unknown validation method: {validation_method}')
         return 11  # Custom error code for unknown validation method
 
-    # Handle validation result
-    if not rng_success:
-        console_err.print(
-            f'{shortname:<{max_len}}: RNG validation => [red]failed[/red]'
-        )
-        if rng_output:
-            console_err.print(f'  [bold red]Error:[/] {rng_output}')
-        return 10  # Specific error code for RNG failure
+    # Handle validation result for jing
+    if validation_method == 'jing':
+        if jing_result.returncode != 0:
+            console_err.print(
+                f'{shortname:<{max_len}}: RNG validation => [red]failed[/red]'
+            )
+            output = (jing_result.stdout or '') + (jing_result.stderr or '')
+            if output:
+                console_err.print(f'  [bold red]Error:[/] {output.strip()}')
+            return 10  # Specific error code for RNG failure
+
+    # Handle validation result for lxml
+    if validation_method == 'lxml':
+        if not rng_success:
+            console_err.print(
+                f'{shortname:<{max_len}}: RNG validation => [red]failed[/red]'
+            )
+            if rng_output:
+                console_err.print(f'  [bold red]Error:[/] {rng_output}')
+            return 10
 
     # 2. Python-based checks
     try:

tests/cli/cmd_validate/test_process.py

@@ -35,7 +35,10 @@ def mock_context() -> DocBuildContext:
 
 
 @patch.object(process_module, 'validate_rng',
-              new_callable=AsyncMock, return_value=(True, ''))
+              new_callable=AsyncMock,
+              return_value=CompletedProcess(args=['jing'],
+                                            returncode=0, stdout='', stderr='')
+)
 @patch.object(process_module.etree, 'parse', side_effect=ValueError('Generic test error'))
 async def test_process_file_with_generic_parsing_error(
     mock_etree_parse, mock_validate_rng, mock_context, tmp_path, capsys
@@ -129,10 +132,10 @@ async def test_validate_rng_with_idcheck_success(mock_run_command, tmp_path):
     rng_schema = tmp_path / 'schema.rnc'
     rng_schema.touch()
 
-    is_valid, message = await validate_rng(xml_file, rng_schema, xinclude=False, idcheck=True)
+    proc = await validate_rng(xml_file, rng_schema, xinclude=False, idcheck=True)
 
-    assert is_valid is True
-    assert message == ''
+    assert proc.returncode == 0
+    assert proc.stdout == '' and proc.stderr == ''
     # Ensure -i flag is passed to jing
     assert "-i" in mock_run_command.call_args.args[0]
 
@@ -151,10 +154,10 @@ async def test_validate_rng_with_idcheck_duplicate_failure(mock_run_command, tmp
     rng_schema = tmp_path / 'schema.rnc'
     rng_schema.touch()
 
-    is_valid, message = await validate_rng(xml_file, rng_schema, xinclude=False, idcheck=True)
+    proc = await validate_rng(xml_file, rng_schema, xinclude=False, idcheck=True)
 
-    assert is_valid is False
-    assert 'duplicate ID' in message
+    assert proc.returncode != 0
+    assert 'duplicate ID' in proc.stderr
     # Ensure -i flag is passed to jing
     assert '-i' in mock_run_command.call_args.args[0]
 
@@ -173,9 +176,9 @@ async def test_validate_rng_without_idcheck_success(mock_run_command, tmp_path):
     rng_schema = tmp_path / 'schema.rnc'
     rng_schema.touch()
 
-    is_valid, message = await validate_rng(xml_file, rng_schema, xinclude=False, idcheck=False)
+    proc = await validate_rng(xml_file, rng_schema, xinclude=False, idcheck=False)
 
-    assert is_valid is True
-    assert message == ''
+    assert proc.returncode == 0
+    assert proc.stdout == '' and proc.stderr == ''
     # Ensure -i flag is NOT passed to jing
     assert '-i' not in mock_run_command.call_args.args[0]

tests/cli/cmd_validate/validate/test_cmd_validate.py

@@ -4,6 +4,7 @@
 from os import PathLike
 from pathlib import Path
 import tempfile
+from subprocess import CompletedProcess
 from unittest.mock import AsyncMock, Mock, patch
 
 import pytest
@@ -161,7 +162,7 @@ async def test_process_check_exception(
         self, mock_validate_rng, mock_registry, mock_context, valid_xml_file: PathLike
     ):
         """Test processing when a check raises an exception."""
-        mock_validate_rng.return_value = (True, '')
+        mock_validate_rng.return_value = CompletedProcess(args=['jing'], returncode=0, stdout='', stderr='')
         # Create a mock check that raises an exception
         mock_check = Mock()
         mock_check.__name__ = 'failing_check'
@@ -178,7 +179,7 @@ async def test_process_successful_checks(
         self, mock_validate_rng, mock_registry, mock_context, valid_xml_file
     ):
         """Test processing with successful checks returns 0."""
-        mock_validate_rng.return_value = (True, '')
+        mock_validate_rng.return_value = CompletedProcess(args=['jing'], returncode=0, stdout='', stderr='')
         # Create a mock check that succeeds
         mock_check = Mock()
         mock_check.__name__ = 'passing_check'
@@ -195,7 +196,7 @@ async def test_process_failed_checks(
         self, mock_validate_rng, mock_registry, mock_context, valid_xml_file
     ):
         """Test processing with failed checks returns 1."""
-        mock_validate_rng.return_value = (True, '')
+        mock_validate_rng.return_value = CompletedProcess(args=['jing'], returncode=0, stdout='', stderr='')
         # Create a mock check that fails
         mock_check = Mock()
         mock_check.__name__ = 'failing_check'
@@ -211,7 +212,7 @@ async def test_process_shortname_generation(
         self, mock_validate_rng, mock_context, valid_xml_file
     ):
         """Test that shortname is generated correctly for display."""
-        mock_validate_rng.return_value = (True, '')
+        mock_validate_rng.return_value = CompletedProcess(args=['jing'], returncode=0, stdout='', stderr='')
         with patch.object(process_mod, 'registry') as mock_registry:
             mock_registry.registry = []
 
@@ -226,7 +227,7 @@ async def test_process_with_multiple_files_and_iterator(
         self, mock_validate_rng, mock_context, valid_xml_file
     ):
         """Test that processing multiple files works with an iterator."""
-        mock_validate_rng.return_value = (True, '')
+        mock_validate_rng.return_value = CompletedProcess(args=['jing'], returncode=0, stdout='', stderr='')
         with patch.object(process_mod, 'registry') as mock_registry:
             mock_registry.registry = []
 
@@ -241,7 +242,7 @@ async def test_process_stitch_validation_fails_on_duplicates(
         self, mock_validate_rng, mock_context, tmp_path, capsys
     ):
         """Test `process` fails if stitch validation finds duplicate product IDs."""
-        mock_validate_rng.return_value = (True, '')
+        mock_validate_rng.return_value = CompletedProcess(args=['jing'], returncode=0, stdout='', stderr='')
 
         file1: Path = tmp_path / 'file1.xml'
         file1.write_text('<product productid="sles"><docset setid="15-sp4"/></product>')

tests/cli/cmd_validate/validate/test_process_validation.py

@@ -37,9 +37,9 @@ async def test_validate_rng_with_rng_suffix(tmp_path: Path):
   <start><element name="root"><text/></element></start>
 </grammar>""")
 
-    returncode, _ = await process_mod.validate_rng(xmlfile, rng_schema)
+    proc = await process_mod.validate_rng(xmlfile, rng_schema)
 
-    assert returncode, f'Expected True, got {returncode}'
+    assert proc.returncode == 0, f'Expected return code 0, got {proc.returncode}'
 
 
 @pytest.mark.skipif(not is_jing_installed(), reason="jing command not found")
@@ -54,9 +54,9 @@ async def test_validate_rng_with_invalid_xml(tmp_path: Path):
     <start><element name="root"><text/></element></start>
 </grammar>""")
 
-    returncode, message = await process_mod.validate_rng(xmlfile, rng_schema)
-    assert returncode is False, f'Expected False, got {returncode}'
-    assert 'error: element "wrong_root"' in message
+    proc = await process_mod.validate_rng(xmlfile, rng_schema)
+    assert proc.returncode != 0, f'Expected non-zero return code, got {proc.returncode}'
+    assert 'error: element "wrong_root"' in (proc.stdout + proc.stderr)
 
 
 @pytest.mark.skipif(not is_jing_installed(), reason="jing command not found")
@@ -71,9 +71,9 @@ async def test_validate_rng_without_xinclude(tmp_path: Path):
     <start><element name="root"><text/></element></start>
 </grammar>""")
 
-    returncode, _ = await process_mod.validate_rng(xmlfile, rng_schema, xinclude=False)
+    proc = await process_mod.validate_rng(xmlfile, rng_schema, xinclude=False)
 
-    assert returncode, f'Expected True, got {returncode}'
+    assert proc.returncode == 0, f'Expected return code 0, got {proc.returncode}'
 
 
 @pytest.mark.skipif(not is_jing_installed(), reason="jing command not found")
@@ -88,12 +88,10 @@ async def test_validate_rng_with_invalid_xml_without_xinclude(tmp_path: Path):
     <start><element name="root"><text/></element></start>
 </grammar>""")
 
-    returncode, message = await process_mod.validate_rng(
-        xmlfile, rng_schema, xinclude=False
-    )
+    proc = await process_mod.validate_rng(xmlfile, rng_schema, xinclude=False)
 
-    assert returncode is False, f'Expected False, got {returncode}'
-    assert 'element "wrong_root" not allowed anywhere' in message
+    assert proc.returncode != 0, f'Expected non-zero return code, got {proc.returncode}'
+    assert 'element "wrong_root" not allowed anywhere' in (proc.stdout + proc.stderr)
 
 
 async def test_validate_rng_jing_failure():
@@ -112,12 +110,12 @@ async def test_validate_rng_jing_failure():
                       stdout='Error in jing',
                       stderr=''))
     ) as mock_run_command:
-        success, output = await process_mod.validate_rng(
+        proc = await process_mod.validate_rng(
             xmlfile, rng_schema_path=rng_schema, xinclude=False, idcheck=False
         )
 
-        assert not success, 'Expected validation to fail.'
-        assert output == 'Error in jing', f'Unexpected output: {output}'
+        assert proc.returncode != 0, 'Expected validation to fail.'
+        assert proc.stdout == 'Error in jing', f'Unexpected stdout: {proc.stdout}'
 
         mock_run_command.assert_called_once_with(
             ['jing', str(rng_schema), str(xmlfile)]
@@ -137,12 +135,12 @@ async def test_validate_rng_command_not_found():
     with patch.object(
         process_mod, 'run_command', new_callable=AsyncMock, side_effect=error
     ):
-        success, output = await process_mod.validate_rng(
+        proc = await process_mod.validate_rng(
             xmlfile, rng_schema_path=rng_schema, xinclude=False
         )
 
-    assert not success, 'Expected validation to fail.'
-    assert output == 'jing command not found. Please install it to run validation.'
+    assert proc.returncode != 0, 'Expected validation to fail.'
+    assert proc.stderr == 'jing command not found. Please install it to run validation.'
 
 
 async def test_validate_rng_command_not_found_no_filename():
@@ -155,21 +153,21 @@ async def test_validate_rng_command_not_found_no_filename():
     with patch.object(
         process_mod, 'run_command', new_callable=AsyncMock, side_effect=error
     ):
-        success, output = await process_mod.validate_rng(
+        proc = await process_mod.validate_rng(
             xmlfile, rng_schema, xinclude=False
         )
 
-    assert not success, 'Expected validation to fail.'
+    assert proc.returncode != 0, 'Expected validation to fail.'
     assert (
-        output == 'xmllint/jing command not found. Please install it to run validation.'
+        proc.stderr == 'xmllint/jing command not found. Please install it to run validation.'
     )
 
 
 async def test_process_file_with_validation_issues(capsys, tmp_path):
     with patch.object(
         process_mod,
         'validate_rng',
-        new=AsyncMock(return_value=(False, 'Validation error')),
+        new=AsyncMock(return_value=CompletedProcess(args=['jing'], returncode=1, stdout='', stderr='Validation error')),
     ) as mock_validate_rng:
         dir_path = tmp_path / 'path' / 'to'
         dir_path.mkdir(parents=True)
@@ -207,7 +205,7 @@ async def test_process_file_with_xmlsyntax_error(capsys, tmp_path):
             ),
         ) as mock_etree_parse,
         patch.object(
-            process_mod, 'validate_rng', new=AsyncMock(return_value=(True, ''))
+            process_mod, 'validate_rng', new=AsyncMock(return_value=CompletedProcess(args=['jing'], returncode=0, stdout='', stderr=''))
         ) as mock_validate_rng,
     ):
         result = await process_mod.process_file(xmlfile, mock_context, 40)