kriegaex commented on PR #133:
URL: https://github.com/apache/xalan-java/pull/133#issuecomment-1835237583
> I would trade "slightly less test coverage" for "less maintenance overhead
of dealing with Mockito tests/upgrades vs Java versions".
I would not. The right thing to do would be to
* introduce a `xalan-commons` module (there is a lot more duplicate code
in Xalan vs. Serializer modules),
* put a singleton-style versions base class with non-static methods
(easily testable) there, the static ones only being facades for calling the
singleton instance's ones,
* making sure that tests can inject a testable instance.
But that is for another day. Static methods or not, a proper unit test
should cover all code and stub out file I/O, i.e. one way or another we end up
with a partial mock, if we want clean in-memory tests. Especially with
parametrised tests, I do not wish to store a whole bunch of test resource files
for different test cases. Trading coverage for slightly simpler tests is an
option, but not the one I chose for good reason.
> > I downgraded from Mockito 5.x to 4.x to cater to the requirement to be
able to build on JDK 8
>
> An alternative solution could be skipping some of the tests when running
with JDK 8.
I prefer my alternative, not skipping tests. We would have to skip **all**
tests using mockito on JDK 8 then, but good unit tests ought to test the unit
under test in isolation, i.e. I want to use mocks in many situations. This
class is not complex, but others are and have dependencies that I want to mock
when injecting them.
@jkesselm, mocking might be over-used, but it does not mean that I do. Mocks
or test doubles in general are important tools for good unit tests.
> > Now, the CI build clean verify passes
>
> It would be great to actually see the CI results
I checked them, so they were visible already. Because of the strange policy
that each single CI run has to be manually approved by a committer, I simply
checked the results on my fork, from where I started the PR. There, the tests
run automatically. You can look there, too. In case of your own PIs, that is
where you can also verify the build results, if you are not a committer (I have
no idea, actually).
> and it would be great to see CI on both Windows and Linux machines as I
expect there might be newline/pathseparator-like issues
I agree, but it was way beyond the scope of this PR to add a build matrix
here. Actually, OS matrix is not the only thing. We also want a JDK version
matrix. I suggest 8, 11, 17, 21.
--
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]
