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

Fix ScalaMock for SJS 1.x #362

Merged
merged 9 commits into from
Nov 22, 2020
Merged

Fix ScalaMock for SJS 1.x #362

merged 9 commits into from
Nov 22, 2020

Conversation

ddworak
Copy link
Contributor

@ddworak ddworak commented Nov 20, 2020

Pull Request Checklist

  • I agree to licence my contributions under the MIT licence
  • I have added copyright headers to new files
  • I have added tests for any changed functionality

Purpose

Hi! During the migration to SJS 1.x in one of our projects, I've noticed practically none of our mocks made with ScalaMock work, failing with a runtime error. You can replicate the issue on my branch at the point where I enabled JS testing (def240d) by running scalamockJS/test:

java.lang.IllegalArgumentException: Property 'mock$oneParamMethod$0$1' is not defined in 
'org.scalamock.test.specs2.FixtureContextTest$TestSetup$$anon$1@b'. 
Available properties: 
Lorg_scalamock_test_specs2_FixtureContextTest$TestSetup$$anon$1__f_mock$special$mockName,
Lorg_scalamock_test_specs2_FixtureContextTest$TestSetup$$anon$1__f_mock$polymorphicMethod$0,
Lorg_scalamock_test_specs2_FixtureContextTest$TestSetup$$anon$1__f_mock$oneParamMethod$0,
Lorg_scalamock_test_specs2_FixtureContextTest$TestSetup$$anon$1__f_mock$noParamMethod$0,
Lorg_scalamock_test_specs2_FixtureContextTest$TestSetup$$anon$1__f_bitmap$init$0 
(/code/ScalaMock/js/target/scala-2.12/scalamock-test-fastopt/main.js:198250)

After a bit of digging it turned out the JS property names are generated differently and no longer match the implementation in org.scalamock.clazz.MockFunctionFinderImpl.

This PR:

  • (re?)enables unit tests for shared and scalamockJS - scalatest and specs were not included, so no suites were ran at all
  • moves incompatible (mostly due to missing concurrency classes from stdlib or reflection usage) suites to the JVM-only module
  • adds a @JSExport annotation to generated mock methods
  • simplifies the "reflection" algorithm for SJS, which now resembles the JVM counterpart more

Background Context

  • I initially used scalajs-stubs to provide JSExport uniformly, but it didn't work on 2.11 and a check at the macro level was not too complex
  • the JS property name generation algorithm is available in the SJS emitter code - I've decided not to try to imitate it, as such approach turns out to be brittle

@ddworak ddworak changed the title Fix Scalamock for SJS 1.x Fix ScalaMock for SJS 1.x Nov 20, 2020
build.sbt Show resolved Hide resolved
@barkhorn
Copy link
Collaborator

great PR, much appreciated! I've never worked myself with scala.js so the compatibilty has always been a bit dodgy. I've trusted CI to flag up problems, but are you saying it doesn't actually run these?
I could add an explicit goal in the script to force that then

@barkhorn barkhorn added this to the v5.1.0 milestone Nov 22, 2020
@ddworak
Copy link
Contributor Author

ddworak commented Nov 22, 2020

I'm glad you like it. I think you don't need to introduce any further steps to the CI, as scalamock/test always triggered scalamockJS/test - the problem was that there were neither ScalaTest nor specs2 JARs for Scala.js included, so no suites were discovered. This is now fixed with the %%% notation. You can see that the check for this PR ran shared suites twice, once on the JVM and once on node: https://travis-ci.org/github/paulbutcher/ScalaMock/jobs/744832811. That's why I had to move some of the suites to the JVM-only part, as they included reflection or concurrency mechanisms not available in the JS environment.

@barkhorn barkhorn merged commit ff3e26d into paulbutcher:master Nov 22, 2020
@ddworak ddworak deleted the sjs-tests branch November 23, 2020 07:15
@barkhorn
Copy link
Collaborator

barkhorn commented Dec 6, 2020

I've pushed v5.1.0 to maven central in case you want to update

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

Successfully merging this pull request may close these issues.

2 participants