karma-runner / maven-karma-plugin

Maven plugin for running tests using Karma.
Apache License 2.0
45 stars 26 forks source link

make StartMojo thread-safe. #21

Closed KengoTODA closed 10 years ago

KengoTODA commented 10 years ago

Hi, thanks for your great plugin. It is very useful for me and my team.

Today I have a suggestion to make your plugin thread-safe. This change lets user uses -T option of mvn command. Here is a document which explains requirement to make mojo thread-safe.

In this change there are 2 key points:

Thanks in advance, Kengo TODA

kelveden commented 10 years ago

Thanks @eller86. Looks good to me. However, I'm not familiar with the AnsiConsole library that was added by @fbengrid mainly to solve problems on Windows as I understand it. So, I'll open it up for a couple of days for him/her to comment.

Any comments @fbengrid?

fbengrid commented 10 years ago

The uses of AnsiConsole.systemInstall() was to ensure the proper initialization of the librairy (if i remember right). If the output is still correctly colored under windows with the removal of line, I'm fine with it. Unfortunately, as far as i remembered the only way to have properly colored text outputted by karma under windows was to use this library. The only other options was to disable the colored message of karma to avoid seeing escaped characters in the console.

You could argue that since Thread Safety is a concern here for Parallel Builds (I assume ?) I'm not sure how relevant is the need for colored messages ?

I'll look at the current implementation and see how things could be changed to allow the threadsafe annotation.

Note that if you remove/disable the call to AnsiConsole.systemInstall() you need to remove/disable the associated cleaning method call: AnsiConsole.systemUninstall().

fbengrid commented 10 years ago

I did a quick test by commenting the 2 calls made to AnsiConsole.systemInstall();

And it was working as expected under windows 7, event if the one present in the method resetAnsiConsole() should have been AnsiConsole.systemUninstall() instead.

So i assume you can safely delete the 2 lines from the mojo. If someone report back an issue in the future will look into it.

kelveden commented 10 years ago

Great, thanks for taking a look @fbengrid. I'll merge.

I'll try and get a new release of the plugin out in the next few days.

kelveden commented 10 years ago

1.7 released to Maven Central.

KengoTODA commented 10 years ago

Thanks a lot!