Open stewartHutchins opened 4 years ago
@stewartHutchins The reason the scalatest is not included I think is because we do not want to 'fix' a scalatest version for the user, org.scalatest.tools.Runner works pretty consistently across different scalatest version, so user can include the scalatest version that they are using.
I think it is the same reason this plugin is written in Java, so the end user can choose scala version that they are using.
Yes, this is understandable, however I don't think this addresses issue.
It's been a while, but I think that the core of the issue is not "scalatest should be included with the plugin", but moreso "there are instances where a user may reasonably expect that scalatest is not required to be on the classpath".
This may be when:
(As a side note, although the issue title brings up a valid concern - in retrospect it may not have been the best title for the issue)
@stewartHutchins I submitted a PR to add a noScalaTestIgnore configuration:
https://github.com/scalatest/scalatest-maven-plugin/pull/91
Do you think it can address your issue?
Cheers.
I think the PR probably does solve the issue and makes sense, but I am concerned that it may cause a different issue.
Would junit tests also be skipped if scalatest is not on the classpath and the new flag is enabled?
@stewartHutchins Unless you explicitly disable surefire like mentioned in the guide (I can't recall why it is recommended to disable the surefire):
https://www.scalatest.org/user_guide/using_the_scalatest_maven_plugin
I think the normal junit tests running with surefire will still work. If you want to use ScalaTest's to run junit tests then you'll need to bring in scalatest dependency.
If the scalatest jar is responsible for running junit tests, then I guess the fix might have to be some combination of the message saying "scalatest not found on classpath, please add it", rather than "there were test failures", and possibly skipping invocation of the plugin if there are no tests to run - though this 2nd bit might not be possible
Hi @stewartHutchins , fyi I just staged a 2.1.0-SNAP2 that includes my PR for adding noScalaTestIgnore, and I tried it in your demo repo in this PR:
https://github.com/stewartHutchins/scalatest-maven-plugin-bug-demo/pull/3
Can you see if we are getting nearer to what you trying to achieve? We can still improve it before 2.1.0 final is released.
Thanks.
Hi, yes. I think this is closer. I think checking for the jar on the classpath and the change of error message defiantly adds value - and would consider this alone to resolve this issue.
I can see how noScalaTestIgnore
also adds value to the plugin, however (in my opinion) it could also lead to issues where junit tests are accidentally not run - so, whether this is also included is decision for you and the other maintainers.
(Personally, I would merge the change which changes the error message, then as a separate issue investigate whether it's possible to only run the plugin/invoke the scalatest runner when there is compiled code in target/test-classes
.)
@stewartHutchins Makes sense, in my opinion ideally pure junit tests should be run by other plugin like surefire, but JUnitSuite which is a ScalaTest's suite should be run by ScalaTest runner. Let's see what @bvenners thinks.
By the way, in case you have not noticed, when noScalaTestIgnore is set to true and ScalaTest is not available, there's a log message saying 'Skipped because ScalaTest is not available', may be we should use warning log message for that, to make the log message more obvious.
The issue:
If scalatest is not on the classpath when the scalatest-maven-plugin is executed, the plugin shows this error:
However the Build fails with:
Steps to reproduce:
There are 2 fairly reasonable cases (as far as I can think of) where this could happen:
I have made a repo where both of these issues can be replicated with
mvn clean install
: demo repo.Workaround:
This issue can be circumvented by adding a scalatest dependency at the top level of the user's project, forcing all sub-modules to include scalatest. This is demonstrated on the branch 'hotfix', in the demo repo
Please let me know if any more information is needed.