In general, this sort of thing needs to be submitted as small,
isolated PRs that are focused on exactly one thing. Code review time
is extremely limited. I'm not sure if a non-committer can approve PRs
or not, but if so it would be more helpful to contribute as a code
reviewer than a coder.
I will gladly help out by reviewing the open PRs. While my reviews will
be non-binding, I hope it helps to speed up the approval process for the
active committers.
Gradle or Maven is a non-starter for now. We can consider that only
after the build actually works, tests run, and releases are possible.
I think there might be a misunderstanding here: On my fork, the Gradle
build is already fully functional. It builds the project, executes all
tests, and handles the release packaging according to ASF guidelines.
However, I completely understand if migrating the build system is not a
priority for the upstream project right now.
Lambdas are usually a bad idea. There might be isolated cases where
they're clearer than a simple loop, but that's rare. In any case, I'm
not sure we currently require Java 8. That might be a blocker. I'd
have to check. But whether we do or don't, please don't churn the code
just to introduce the new hotness when the old hotness works perfectly
well.
The lambdas in my fork were exclusively introduced to replace the highly
verbose anonymous classes inside AccessController.doPrivileged(...)
blocks across the SecuritySupport classes. I agree it's mostly cosmetic
to reduce boilerplate, so I am perfectly fine leaving these out of any
PRs to avoid unnecessary code churn.
Microbenchmarks rarely identify anything with a macro effect, but as
long as an optimization doesn't make code more opaque or convoluted,
I'll consider it. However, production benchmarks are much more
convincing.
Production benchmarks are indeed harder to get, but I needed a
systematic baseline to verify that the micro-optimizations (like
skipping redundant array allocations) aren't doing any harm or
convoluting the code.
try-with-resources is useful where it fits, especially on code paths
where resources might not be properly closed already.
I agree. Going forward, if I spot unclosed resources on error paths, I
can submit those as small, isolated try-with-resources PRs.
JUnit 5 is a maybe, but it doesn't add a lot of benefit, and I'd
prefer not to burn time on it. On the other hand, converting non-JUnit
tests to JUnit is very useful, and needs to be completed before we
consider migrating to a new JUnit version. Some of the Xerces codebase
is very old and might predate JUnit 1.0. We have a lot of "tests" that
do not actually run. There are probably still a couple that are
failing but don't break the build. I have over a dozen PRs to fix this
that have been open for months without any reviews.
Exactly this was the core of my test modernization. On my branch, the
legacy test wrappers were migrated to standard JUnit. Additionally, I
implemented JUnit test drivers for the external W3C XMLConformance, DOM,
XInclude, and Schema test suites so they finally run as part of the
normal build.
To give you an idea of the test coverage this unlocks, here is the
complete breakdown of all tests that are now running transparently
during a single build:
Test Suite Category | Executed Tests | Skipped
---------------------------------------|----------------|---------
-- W3C Conformance Suites -- | |
XML Schema Test Suite (XSTS) | 14,030 | 879
XML Conformance Test Suite | 5,170 | 774
DOM Conformance Test Suite (DOM TS) | 1,706 | 120
XInclude 1.0 Conformance Test Suite | 348 | 64
| |
-- Internal Xerces Tests -- | |
Schema (Config, Annotations, etc.) | 155 | 0
DOM (Events, Traversal, Ranges, etc.) | 92 | 0
JAXP (Validation, SecurityProcessing) | 53 | 0
XInclude | 41 | 0
Identity Constraints (IDC) | 27 | 0
Misc (Utilities, Threading, etc.) | 15 | 0
---------------------------------------|----------------|---------
Grand Total | 21,637 | 1,837
(Note: Tests requiring remote network access or containing known spec
inconsistencies within the W3C suites were properly tagged and
conditionally
skipped to ensure a reliable offline build).
TBH there are a lot of real issues and bugs we should address. We get
periodic offers to do work like this that churns code without offering
user value.
I completely understand your point and I really appreciate you taking
the time to give me such detailed feedback! I will focus on reviewing
your open PRs first.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
