vlsi commented on PR #133: URL: https://github.com/apache/xalan-java/pull/133#issuecomment-1833170148
This looks great to me, however, I am not sure it is good to use Mockito here. Sure it would be a great source for copy-pasting the next steps, however, mocking here might be a little bit of overkill. I would rather extract the "version parsing method" to a separate class (e.g. with `String` constructor). Then the parsing itself could be tested without mocks, and the end-to-end could be tested with, well, integration. In other words, I would trade "slightly less test coverage" for "less maintenance overhead of dealing with Mockito tests/upgrades vs Java versions". --- >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. For instance, you could exclude the test file that uses Mockito with `<testExcludes>` and be done with it: https://github.com/pgjdbc/pgjdbc/blob/6f11a2854ef67aec16dd413e2fa88bd8512eaf3d/pgjdbc/reduced-pom.xml#L138-L145 --- > Now, the CI build clean verify passes It would be great to actually see the CI results, and it would be great to see CI on both Windows and Linux machines as I expect there might be newline/pathseparator-like issues -- 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
