vlsi commented on code in PR #129:
URL: https://github.com/apache/xalan-java/pull/129#discussion_r1407195382
##########
serializer/src/main/java/org/apache/xml/serializer/Version.java:
##########
@@ -55,7 +55,16 @@ public final class Version
private static void readProperties() {
Properties pomProperties = new Properties();
- try (InputStream fromResource =
Version.class.getClassLoader().getResourceAsStream(POM_PROPERTIES_PATH)) {
+ ClassLoader classLoader = Version.class.getClassLoader();
+ if (classLoader == null) {
+ // Oops! Someone put Xalan is on the bootstrap class loader (BCL) -> fall
+ // back to the system class loader, because there is no Classloader
+ // instance for the BCL (native code). Due to class loader hierarchy,
+ // however, the resource will also be found when asking for it from a
+ // level below the BCL.
+ classLoader = ClassLoader.getSystemClassLoader();
+ }
+ try (InputStream fromResource =
classLoader.getResourceAsStream(POM_PROPERTIES_PATH)) {
Review Comment:
@kriegaex , let me be very explicit:
1. Please forget about xalan-test for now.
2. Please add junit-jupiter test scope dependency
3. Please create VersionTest.java file and add several test methods there.
The methods should call public methods and verify they return a string that
starts with "2."
4. Please factor regexp that parses version into package-private metod (or
to a separate ParsedVersion class)
5. Please add ParsedVersionTese.java with a parameterized test that verifies
several inputs. It could cover "d" versions or explicitly mention "d" versions
will never be used (sure nobody should release those strange "d" versions now)
The tests I mean are like a couple of files 20 lines each.
Please behave. Your previous message sounds as if I asked you something
complicated, however adding unit tests is trivial. If you see issues with
adding unit tests, please tell me the reason explicitly. Sure it might be
challenging to test "xalan on the boot classloader" with a regular junit
jupiter test, however, a trivial test would still guard from regressions on the
regular classloaders.
The lack of tests should not be your excuse for not adding tests for the
code you change or create.
> why not lend a hand and migrate the test project into this one and thus
eliminate the root cause of the problem?
0) Migrating xalan-test into xalan is not needed for adding unit test. At
the same time, it would be a bad idea to test the regexp, property parsing, and
basic Version features with integration tests
1) My PRs are either ignored or mentioned as "not a priority", so I am not
going to create more until the situation improves
2) I am not interested in spending my time on Maven. Sure I can rework
xalan-test with Gradle. However, I say it here **only because you ask**. I
estimate the idea would not be accepted, so I don't even mention it on the dev
list. At the same time, please note I offer a lot yet the team is just not
willing to accept it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@xalan.apache.org
For additional commands, e-mail: dev-h...@xalan.apache.org
