Skip to content

Conversation

@dsoumis
Copy link
Member

@dsoumis dsoumis commented Aug 29, 2025

  1. Adds TomcatBaseTest.WebappLogCapture to capture logs emitted under the webapp classloader during context start.
  2. Add TestStrictComplianceDeployment for STRICT_SERVLET_COMPLIANCE web.xml parsing. Test whether a SAXException is present in the log when strict servlet compliance turned on in catalina.properties.

@dsoumis dsoumis force-pushed the test-strict-servlet-compliance branch from 3d834cb to 079cc1e Compare August 29, 2025 21:31
@rmaucher
Copy link
Contributor

rmaucher commented Sep 1, 2025

Testing the log contents is not as stable as checking some other things, so it should be avoided if possible. However, sometimes it is useful so adding the capability to the testsuite seems like a good idea to me.

@markt-asf
Copy link
Contributor

Big +1 to expanding the coverage of the unit tests.
I was concerned about setting the system property impacting other tests but that is not an issue for tests run via Ant as each test class is tested in a separate JVM (by default and we haven't changed that). Tests run in the IDE will run in a single JVM but normally it is only single tests (or a package of tests) that are run so that won't normally be an issue.
My other concern is that there are two things being tested here. 1) that setting STRICT_SERVLET_COMPLIANCE enables XML validation and 2) that XML validation works as expected. I think it might be better to split that into two tests. One testing that the XML validation works (configured using the Context attributes) and one testing that setting STRICT_SERVLET_COMPLIANCE sets the attributes it is documented to set.

@rmaucher
Copy link
Contributor

rmaucher commented Sep 1, 2025

Yes, this kind of use of a system property has been shown to cause testsuite unreliability issues in the past. This was indeed mostly a "fail if XML validation is enabled" test.

I think the utility that allows processing logs in the testsuite should be committed first.

@dsoumis
Copy link
Member Author

dsoumis commented Sep 1, 2025

I have split the tests in two, one testing xml validation and the other STRICT_SERVLET_COMPLIANCE sets all the attributes as expected. Test will be skipped now, if the constants have already been initialized under the same JVM. I think this will avert any issues if the test suite is being run through IDE.

@dsoumis dsoumis force-pushed the test-strict-servlet-compliance branch from d051e5b to 8361a8e Compare September 1, 2025 18:11
@dsoumis dsoumis force-pushed the test-strict-servlet-compliance branch from 8361a8e to 377875c Compare September 1, 2025 18:13
@markt-asf
Copy link
Contributor

LGTM. One minor nit is that continuation lines should be indented 8 spaces rather than 4.

@dsoumis dsoumis merged commit 135c54d into apache:main Sep 2, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants