Den tors 25 juni 2026 kl 15:57 skrev Ivan Zhakov <[email protected]>: > On Wed, 24 Jun 2026 at 18:03, Branko Čibej <[email protected]> wrote: > >> On 24. 6. 2026 16:03, Ivan Zhakov wrote: >> >> On Tue, 23 Jun 2026 at 17:42, Daniel Sahlberg < >> [email protected]> wrote: >> >>> Den tis 23 juni 2026 kl 15:34 skrev <[email protected]>: >>> >>>> Author: ivan >>>> Date: Tue Jun 23 13:33:58 2026 >>>> New Revision: 1935586 >>>> >>>> Log: >>>> On 'xml-schema-validation-improvements' branch: >>>> >>>> Always use installed lxml and rnc2rng Python packages to validate XML >>>> output >>>> of `svn --xml` in tests. >>>> >>>> This change also makes these package mandatory for running tests unless >>>> new >>>> option `--disable-xml-schema-validation` is specified. >>>> >>> >>> [...] >>> >>> I had a few minutes to spare this afternoon so I checked this commit and >>> did three tests on Ubuntu 26.04: >>> - With python3-lxml install: make check succceeded >>> - Without python3-lxml installed: make check failed with error >>> messages, this in the summary: >>> [[[ >>> There were some XML validation errors, >>> checking/home/dsg/svn_branches/xml-schema-validation-improvements/tests.log >>> XML: Module lxml.etree not found >>> ]]] >>> - Without python3-lxml: DISABLE_XML_SCHEMA_VALIDATION=true make check >>> succeed. >>> >>> I think this looks good - fails by default but can be made to work. >>> >>> I also tried cmake/ctest but that failed spectacularly with (62 tests >>> failed) or without lxml (61 tests failed!), so I presume something else is >>> wrong. >>> >>> Only thing remaining is to document this. "make check" is only mentioned >>> under "Building from a Tarball". Maybe it deserves a separate section ("D. >>> Running the test suite"). Possibly even restructuring "II. Installation" to >>> separate our three build systems (autoconf/make, vcproj and CMake) and >>> having a separate section on testing under each? Do we do this on the >>> branch or after merging back to trunk? >>> >>> >> Thanks for testing this! >> >> There are a few things that still need to be done: >> >> 1. I learned that we can actually use the builtin Python XML package to >> check the structural validity of XML responses. While this isn't the same >> as fully validating the response against a schema definition, it still lets >> us catch many cases where we produce invalid XML. >> >> In r1935590 I implemented this improvement and I'm now thinking about >> making this mode the default, enabling full schema validation only if >> explicitly requested. This way maintainers that run tests in their >> pipelines can still get an adequate level of checking without being >> required to install additional packages. And we ourselves will enable the >> full validation in GitHub Actions. >> >> >> >> I like this approach, with one small nit – there's no need to explicitly >> request schema validation, just check at runtime if the required modules >> (lxml and rng2rnc) are available and if they are, perform full validation. >> There is/was already code in svntest/main.py to check that. >> >> > Since we now have the always-on XML structural check, I'd say we should > pick between two options: > > A) Make full schema validation implicit when the modules are available. > B) Have an explicit `--enable-xml-schema-validation` switch to enable it. > > I'm fine with either of these approaches. However, the explicit > `--enable-xml-schema-validation` approach has a couple of advantages: > > - It makes test results, such as error messages, predictable and > independent of the environment. > - It gives users a way to check whether their environment is suitable for > full XML schema validation. If a user passes > `--enable-xml-schema-validation` and the modules aren't present, that's an > error rather than a downgrade of the check. > > That said, I'm also okay with the implicit approach (A) that you described. >
I would prefer option (C): Make full schema validation default (failing if lxml/rnc2rng is not available) unless explicitly disabled. (I believe this is as currently implemented on the branch). My argument is that we risk forgetting to run the schema validation tests if they are not enabled. Better to force developers to install the dependencies to be able to run the tests - or use the option to downgrade the check. I believe the lxml and rnc2rng dependencies are relatively minor but I didn't check anything other than Ubuntu. If they are difficult to install on any platform, we can maybe have different default per platform? Cheers, Daniel
