Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mismach of errors between maven and scalatest-maven-plugin, 'There are test failures' VS 'java.lang.ClassNotFoundException' #68

Open
stewartHutchins opened this issue Jan 6, 2020 · 9 comments

Comments

@stewartHutchins
Copy link

stewartHutchins commented Jan 6, 2020

The issue:

If scalatest is not on the classpath when the scalatest-maven-plugin is executed, the plugin shows this error:

Error: Could not find or load main class org.scalatest.tools.Runner
Caused by: java.lang.ClassNotFoundException: org.scalatest.tools.Runner

However the Build fails with:

There are test failures -> [Help 1]

Steps to reproduce:

There are 2 fairly reasonable cases (as far as I can think of) where this could happen:

  • The plugin is executed as part of a top level pom, with no code/tests (and thus there is no need to include any dependencies), however the build pipeline is defined in the top level pom, in order to apply the build pipeline to all sub-modules.
  • The plugin is executed as part of a mixed java and scala project. As the plugin can run junit tests, there may be a purely java module which has no need for scalatest, and only relies on junit.

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.

@cheeseng
Copy link
Contributor

@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.

@stewartHutchins
Copy link
Author

stewartHutchins commented Jun 30, 2022

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:

  • There are no tests, so the plugin should not run, and therefore a scalatest jar should not be required to be on the classpath, e.g with pom packaging.
    or:
  • The plugin should run, but the tests are not dependent on scalatest, and should therefore not require a scalatest jar, e.g. if the plugin is running only junit tests.

(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)

@cheeseng
Copy link
Contributor

cheeseng commented Jul 2, 2022

@stewartHutchins I submitted a PR to add a noScalaTestIgnore configuration:

#91

Do you think it can address your issue?

Cheers.

@stewartHutchins
Copy link
Author

stewartHutchins commented Jul 2, 2022

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?

@cheeseng
Copy link
Contributor

cheeseng commented Jul 3, 2022

@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.

@stewartHutchins
Copy link
Author

stewartHutchins commented Jul 3, 2022

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

@cheeseng
Copy link
Contributor

cheeseng commented Jul 6, 2022

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:

stewartHutchins/scalatest-maven-plugin-bug-demo#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.

@stewartHutchins
Copy link
Author

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.)

@cheeseng
Copy link
Contributor

cheeseng commented Jul 6, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants