vlsi commented on PR #133:
URL: https://github.com/apache/xalan-java/pull/133#issuecomment-1835564352
> We would have to skip all tests using mockito on JDK 8 then
Right you are. Of course, it is too early to predict how many Mockito-using
tests will be there in Xalan. At the same time, I estimate there will be a
sheer amount of tests that do not use mocks, so they would be pretty much
runnable with JDK8.
In the current case, the same test coverage can be achieved without Mockito
(see below), and it would not require much effort.
Yet another possibility might be having different test dependency versions
for different JDK versions. That is, of course, a weak idea for API-rich
libraries, however, it might still allow running some of the tests with
multiple JDKs.
> The version parsing method already is separate, and there is a separate
test for it, which does not need mocks. I.e., I already did what you suggested
before you even asked for it. Please check again
You are right. Let me clarify what I meant.
Currently, you have `static void parseVersionNumber(String version) {`, and
you have `VersionAccessor` to access that method.
Sure it works, and I have no issues with that.
I meant a slightly different design (sure it would be generated boilerplate
with Java 8, but it does not change the overall picture):
```java
/* non-public or semi-public */ record XalanVersionRecord(
int major, int release, int maintenance,
int development,
boolean snapshot
) {
XalanVersionRecord(String versionString) {
this(versionString.parseIntoVersionFields());
}
}
```
`Xalan.java` would be like `public static int getMajorVersionNum() { return
INSTANCE.major(); }`
In other words, you keep the old class like it is with its static methods,
however, you could move all the parsing logic to a regular non-static class, so
you can have nice final fields there, and you can have nice constructors.
I think having a struct with all final fields is a little bit easier to
comprehend than having multiple mutable static fields + `static void
resetVersionNumber` and things like that.
If you add `XalanVersionRecord(InputStream ..)` constructor, then you no
longer need mocks in `testReadProperties`.
I see you have a test that verifies "NPE from getPropertiesStream", however,
frankly speaking, I think the test does not add much to the test coverage.
Sure the current code works well.
Of course, you can add `InputStream` parameter to your current `static
readVersionNumber` method, and it would eliminate the need for the mock.
Of course, there are cases when mocks are helpful. I just mean in the
current case, Mockito does add much, and, well, adding more complicated mocks
would be a definite overkill for testing versions.
----
@kriegaex , the Maven part of the build prints 5900 messages which makes it
hard to scroll and understand.
What is the use of messages like "Generating
/home/runner/work/xalan-java/xalan-java/target/site/apidocs"?
What is the purpose of messages like "Loading source files for package
org.apache.xalan.res"?
What is the purpose of "Warning: Entry:
xalan-project_2.7.3/docs/apidocs/org/apache/xml/serializer/utils/class-use/SerializerMessages_zh_TW.html
longer than 100 characters."? Sure file paths can exceed 100 characters, and I
do not see why print such a message.
CI prints a stack trace even though the tests pass:
https://github.com/apache/xalan-java/actions/runs/7055388836/job/19206140606?pr=133#step:4:2885
Can you somehow make it avoid printing the error in case the exception was
intentional?
Of course, we want negative tests, however, I doubt that we want to print
stack traces in case everything works smoothly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
