Rework MavenConsoleLineTracker

[HannesWell]

This PR has the goal to improve the experience with the functionality provided by the MavenConsoleLineTracker and is a preparatory step for PR #471. It is a major rework of the MavenConsoleLineTracker and addresses the following points:

• Improvements for Debugger-Connection:

  • launch Remote-Java-Application immediately when a forked VM waiting for a debugger-connection is detected instead of only creating a link
  • fix debug listening detection (and port computation) when lines start with [INFO]
  • set the project under test as project of the debug-launch (makes sources immediately available)

• Improvements for Test-Report links:

  • speed up search for test-report files (search only in project's target folder, not in entire working-dir)
  • fix test-report link opening when baseDir contains variables
  • fix test-name computation and test-report link alignment when lines start with [INFO]

• add tests for test-report links and automatic debugger attachment

[HannesWell]

I wonder what kind of lines this Pattern is intended to match? Lines that contain any fully qualified class name that starts with test and is prefixed with two spaces?
If yes the Pattern seems to be wrong, because I suspect the first dot should have been escaped, so the following capturing group likely only captured one symbol.
Furthermore if the fully-qualified class name starts with test. one will not find it without that first name element.

For me it looks like it would only work if a Maven-plugin prints test.+test-class-name for a executed test-class.
Does anybody know such plugin?
If not, I suggest to remove that Pattern entirely.

[mickaelistria]

I believe this pattern is meant to capute testMethod (starting by test, .+ being the other names of the method, and the other part capturing paramters. This is probably used in order to like to method when some test failures are reported.
IMO, we should keep it, however we should improve it (later) so that it doesn't rely on the assumption that test methods start by test.

[laeubi]

Just a general question: Why do we need to parse the console output? Should the surefire reports not be a better source for actual test classes names?

[mickaelistria]

Some people like to run Maven builds from the IDE, and those builds may include tests, and in such case, it's convenient to link class names to failed methods directly to the console so people can see in 1 click where the failure happens.

[laeubi]

Sure but shouldn't we parse the names of the test from the results directory and then match those to the console?

[mickaelistria]

Sure but shouldn't we parse the names of the test from the results directory and then match those to the console?

Maybe ;)
IMO this PR does already much, maybe we should split it into separate topics. @HannesWell do you think you can have a separate PR for the test report parts?

[laeubi]

IMO this PR does already much, maybe we should split it into separate topics.

Yes, this was just something that came into my mind about that "magic regexp" stuff might better be handled explicitly.

[HannesWell]

Some people like to run Maven builds from the IDE, and those builds may include tests, and in such case, it's convenient to link class names to failed methods directly to the console so people can see in 1 click where the failure happens.

That's right. I find this very handy, especially with the improvements intended to introduce with this change that the JUnit-View is opened to display the test report. Furthermore it significantly simplifies debugging of forked VMs (like for Unit-tests) because the launch of the debugger is setup and (from now on) started automatically. With PR #471 this should become even simpler and debugging forked is as simple as debugging regular applications.

Sure but shouldn't we parse the names of the test from the results directory and then match those to the console?

Which names are you referring to exactly? To the names of failed tests or to the names of Running tests?
For the latter I expect we would lose more than we gain. The content of the console would have to be searched afterwards, which in best case the same effort in code/computation-power compared to now, if this would be done for each report with a fixed name the runtime would become probably squared instead of linear.
Furthermore a.t.m. the target-directory is only searched for report files when the link is activated. If the links would be created based on the Junit-report files the file-system would have to be searched for them in any case while the build is running. And from my experience file-system access is very expensive compared to other operations. A lazy behavior is likely more performant in most cases.

Nevertheless I think it is reasonable to parse at least the current's project GAV from the console to then query it from the project registry, because it allows to restrict the search-space for test-report files (when ever they are searched). I tried to make the read of those project information as performant as possible. Additionally it will allow for example to put links to a project's pom into the console, which I think could be a handy feature in the future.

However we could consider to further restrict the search space for test-reports in the target-folder if we expect that the surefire/failssafe (and the tycho pendants) never change their reportDirectory (or it could be read from the project configuration). But I'm not sure if this is necessary.

Sure but shouldn't we parse the names of the test from the results directory and then match those to the console?

Maybe ;) IMO this PR does already much, maybe we should split it into separate topics. @HannesWell do you think you can have a separate PR for the test report parts?

I'm about to restore the TEST_CLASS_PATTERN to revert the part to search for the JUnit-XML reports too.
Nevertheless I would like to keep the improved "search-root" as it is because IMHO it makes no sense to start a search from the working directory if the path to the target-folder of the project in question is available and the search space can be much smaller.
I will open a second PR with those changes where we can further discuss the search for test reports.

[HannesWell]

If you are fine with this change as it is, I will squash-and-merge it.

[mickaelistria]

+1 to squash and merge now.

[HannesWell]

Thanks for the discussion.