carlosame
commented
4 years ago the
pom.xmlinvokes the AntRunjavacandjavatargets withfork=true.
With which JDK version are you running the tests?
Closed sideshowbarker closed 3 years ago
carlosame
commented
4 years ago the
pom.xmlinvokes the AntRunjavacandjavatargets withfork=true.
With which JDK version are you running the tests?
carlosame
commented
4 years ago With which JDK version are you running the tests?
Nevermind, I see the [8, 11, 14]. It is unclear what the outcome with 11 and 14 is going to be once the project is modularized, and the fork could make things a bit more 'interesting' in that case.
Is the fork also necessary with JDK 11 and 14?
sideshowbarker
commented
4 years ago Is the
forkalso necessary with JDK 11 and 14?
I don’t know. It might not be. I can test.
The reason I don’t know yet is that apparently when you have series of jobs to run in GitHub Actions, and the first couple of jobs fail, the remaining jobs get canceled.
So to test whether we’d have the same problem without fork under JDK 11 and 14, maybe I just need to change the config to have [11, 14, 8] (to make the Java 8 jobs run last).
anthonyvdotbe
commented
4 years ago My advice would be to use "the Maven way", i.e. adapt the tests such that the Surefire plugin can run them (it's possible to do so without adopting a test framework, as explained here).
sideshowbarker
commented
4 years ago My advice would be to use "the Maven way", i.e. adapt the tests such that the Surefire plugin can run them
Yeah that would seem to be the better way to do it in the long term — but for the near term it seems good first get something working well enough that we just actually run the tests and see what’s failing, and make any fixes needed to the parser sources to get the tests passing.
(it's possible to do so without adopting a test framework, as explained here)
Thanks — yeah, I personally have zero experience with JUnit (and have never even heard of TestNG…), so it’s encouraging to know we at least wouldn’t have to shoehorn the test harness into one of those just in order to be able to use Surefire.
anthonyvdotbe
commented
4 years ago Please have a look at this branch. It's #43, rebased onto #44, with 4 additional commits:
Html5libTest, which follows the "POJO test" conventions (with minimal changes to other files)verify instead of package)So I wouldn't bother with AntRun, since achieving this with Surefire is way easier.
sideshowbarker
commented
4 years ago Please have a look at this branch. It's #43, rebased onto #44, with 4 additional commits:
anthonyvdotbe
commented
4 years ago Are you running with JDK 11+? I've been using JDK 14 myself.
sideshowbarker
commented
4 years ago Are you running with JDK 11+? I've been using JDK 14 myself.
$ java --version
openjdk 14.0.1 2020-04-14
OpenJDK Runtime Environment (build 14.0.1+14)
OpenJDK 64-Bit Server VM (build 14.0.1+14, mixed mode, sharing)My bad, it doesn't work indeed. Not sure what happened there, I'll look into it.
sideshowbarker
commented
4 years ago My bad, it doesn't work indeed. Not sure what happened there, I'll look into it.
OK. Anyway, it looks promising. Would definitely prefer to do it this way if we can make it work.
One thing I notice is this:
[INFO] Running nu.validator.htmlparser.test.DomTest I’m not sure if that’s causing the failure, but I think we don’t actually want to be trying to run that class. Instead I think we want to only be running Html5libTest.
anthonyvdotbe
commented
4 years ago It's because the submodule has moved, so you need to update it. After doing so, it works as expected (where "expected" in this case means: the build fails with test failures).
sideshowbarker
commented
4 years ago It's because the submodule has moved, so you need to update it. After doing so, it works as expected (where "expected" in this case means: the build fails with test failures).
Ah, OK — will try that now
anthonyvdotbe
commented
4 years ago One thing I notice is this:
[INFO] Running nu.validator.htmlparser.test.DomTestI’m not sure if that’s causing the failure, but I think we don’t actually want to be trying to run that class. Instead I think we want to only be running
Html5libTest.
That's because the class name happens to end with "Test". But since it doesn't contain any methods that begin with "test", it doesn't really do anything.
sideshowbarker
commented
4 years ago That's because the class name happens to end with "Test". But since it doesn't contain any methods that begin with "test", it doesn't really do anything.
ah yeah OK
sideshowbarker
commented
4 years ago It's because the submodule has moved, so you need to update it. After doing so, it works as expected (where "expected" in this case means: the build fails with test failures).
OK, did the submodule update and can now confirm that it’s working as expected in my environment.
This is a big improvement — thanks much
I guess now I’ll work on cherry-picking the test changes over from your branch to the #44 branch
PR #44 includes a change to the Maven
pom.xmlfile to enable automated testing of the html5lib-tests suite. See 2fbcd75.This issue is a request for review of that change by anyone with domain expertise/experience with Maven — in particular, with the AntRun plugin.
You’ll notice the
pom.xmlinvokes the AntRunjavacandjavatargets withfork=true. There are comments in the change which explain why, but ideally, it seems preferable to make it work without needingfork=true. So any insights into how that might be possible would be welcome.@anthonyvdotbe and @carlosame — your reviews would be especially welcome.