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

error when mocking a Scala.js native method #191

Open
wgillett opened this issue Jul 6, 2017 · 16 comments
Open

error when mocking a Scala.js native method #191

wgillett opened this issue Jul 6, 2017 · 16 comments

Comments

@wgillett
Copy link

wgillett commented Jul 6, 2017

If you want to discuss a new feature, please ignore/clear this form.

ScalaMock Version (e.g. 3.5.0)

3.5.0

Scala Version (e.g. 2.12)

2.11.8

Runtime (JVM or JS)

JS

Please describe the expected behavior of the issue

Mocking the CanvasRenderingContext2D class should yield a mock object

Please provide a description of what actually happens

Compilation failure:

Error:(18, 24) When overriding a native method with default arguments, the overriding method must explicitly repeat the default arguments.
val canvas = mock[CanvasRenderingContext2D]

Reproducible Test Case

Here's a really simple test case that won't compile (yields the error shown above, with different row/col values because not the same code as used above). Note that this test case works fine with Mockito, so I'll switch to Mockito for now, but would love to be able to use ScalaMock.

import org.scalajs.dom.CanvasRenderingContext2D
import org.scalamock.scalatest.MockFactory
import org.scalatest.FunSpec

class BadSpec extends FunSpec with MockFactory {
  describe("Bad") {
    it("makes a mock") {
      val canvas = mock[CanvasRenderingContext2D]
    }
  }
}
@barkhorn
Copy link
Collaborator

barkhorn commented Jul 7, 2017

Thanks, will take a look. Runtime says JVM, but your report is about the JS backend if i read the test case right? Also, there's ScalaMock 3.6.0 now, but doubt that would change anything, given the error message you reported.

@wgillett
Copy link
Author

wgillett commented Jul 7, 2017

My bad, this is a Scala.js project so should have said JS, just fixed that. Thanks for looking at this.

@barkhorn barkhorn added the ready label Jul 8, 2017
@barkhorn
Copy link
Collaborator

barkhorn commented Jul 8, 2017

Added libraryDependencies += "org.scala-js" %%% "scalajs-dom" % "0.9.3" to pull in CanvasRenderingContext2D and get the same error on ScalaMock master.

@barkhorn
Copy link
Collaborator

barkhorn commented Jul 8, 2017

Could you try to create an adaptor between your code and the native class?

E.g. like this:

// stub to define the functions you intend to use from the scala-js native class
  trait CanvasAdaptor {
    def fillText(text: String, x: Double, y: Double, maxWidth: Double): Unit
  }

// use this in the production wiring
class RealCanvas extends CanvasRenderingContext2D with CanvasAdaptor
  
// and mock the trait in tests
class FooTest extends FunSpec with MockFactory {
  describe("Bad") {
    it("makes a mock") {
      val canvas = mock[CanvasAdaptor]
    }
  }
}

Been playing around a bit, but I fear a fix may be shorlived when Scala-js moves to 1.0 and prevents overrides, so I am not sure if it is fixable from a library PoV. Will continue to play, but maybe the workaround is fine?

      class MyCanvasRenderingContext2D extends CanvasRenderingContext2D {
        override def putImageData(imagedata: ImageData, dx: Double, dy: Double, dirtyX: Double, dirtyY: Double, dirtyWidth: Double, dirtyHeight: Double) = ???

        override def fillText(text: String, x: Double, y: Double, maxWidth: Double) = ???

        override def createImageData(imageDataOrSw: js.Any, sh: Double) = ???

        override def clip(fillRule: String) = ???

        override def drawImage(image: HTMLElement, offsetX: Double, offsetY: Double, width: Double, height: Double, canvasOffsetX: Double, canvasOffsetY: Double, canvasImageWidth: Double, canvasImageHeight: Double) = ???

        override def strokeText(text: String, x: Double, y: Double, maxWidth: Double) = ???
      }
[warn] class MyCanvasRenderingContext2D extends CanvasRenderingContext2D {
[warn]       ^
[warn] .../FooTest.scala:12: Members of traits, classes and objects extending js.Any may only contain members that call js.native. This will be enforced in 1.0.

@barkhorn
Copy link
Collaborator

barkhorn commented Jul 8, 2017

@sjrd or @gzm0 - do you have a view on how to safely subclass things like CanvasRenderingContext2D?

@wgillett
Copy link
Author

wgillett commented Jul 8, 2017

I am not familiar with mocking internals, but I am curious how Mockito solves this problem. Workaround makes sense, thanks @barkhorn. Given there's a workaround, that lowers the priority of this issue, although would certainly be nice if mocking classes like this worked out of the box.

@barkhorn barkhorn added in progress and removed ready labels Jul 8, 2017
@sjrd
Copy link

sjrd commented Jul 9, 2017

First, there is something fishy about the class MyCanvasRenderingContext2D. It doesn't have either an @ScalaJSDefined annotation nor an @js.native annotation. This should emit a warning. It seems you want to use it as @ScalaJSDefined, given that your methods are = ???. But then also, your workaround seems to work with the current default (which is @js.native). So I think what you want is annotate it with @js.native and replace all the method bodies by = js.native, instead of = ???.

@sjrd
Copy link

sjrd commented Jul 9, 2017

I do not really understand what mock[] does to begin with, when the class it's mocking is a native JS class. My intuition is that it should generate an @ScalaJSDefined class extending that class. But yes, default parameters would be extremely tricky to handle. In fact there is no automated way to mock default parameters from a native JS type, because their default value (rhs of the =) is irrelevant and never compiled.

I am not sure what to do about those :-s

@barkhorn
Copy link
Collaborator

barkhorn commented Jul 9, 2017

mock[T] creates an anonymous subclass of T, which is why the mechanisms around js.native are a bit problematic.
From a mock perspective the ideal is a plain trait without implementations, so I suggested overlaying this on the native class as in the example above.
For ScalaMock, I may be able to find a way to add this annotation, but the warning messages suggest that this is only going to be working for scala-js <1.0.
Is there going to be an abstract class/trait in scala-js to decouple from the native, concrete implementation?

@sjrd
Copy link

sjrd commented Jul 9, 2017

mock[T] creates an anonymous subclass of T, which is why the mechanisms around js.native are a bit problematic.

Ah OK. If it is anonymous, then it is automatically @ScalaJSDefined. That explains the original error message, which would only makes sense in an @ScalaJSDefined class.

For ScalaMock, I may be able to find a way to add this annotation, but the warning messages suggest that this is only going to be working for scala-js <1.0.

If you're talking about this warning message:

Members of traits, classes and objects extending js.Any may only contain members that call js.native. This will be enforced in 1.0.

Then the solution to it (both to make the warning disappear and make it 1.0-proof) is to replace the = ??? by = js.native. Really, it's just that.

Is there going to be an abstract class/trait in scala-js to decouple from the native, concrete implementation?

I'm not sure I understand. Things have an abstract class/trait if and only if the library declares an abstract class/trait. Scala.js would not interfere with the library's decision. Are you talking about CanvasRenderingContext2D specifically? If yes, then no there won't be a future version of the library with an abstracted interface. That's defined in scalajs-dom, and adding abstract interfaces to every single native class in there is not sustainable, nor does it match the original Web IDL definitions. It would make everything more obscure for users of that library (i.e., virtually all Scala.js developers).

@barkhorn
Copy link
Collaborator

barkhorn commented Jul 9, 2017

Thanks, makes sense not to introduce abstractions - just wanted to be sure there isn't an easy way out before investing a lot of time..
What that means is that means the macros will have to inspect the tree of the type-to-mock and check if any parameters and/or implementation are defined as js.native, and then repeat those in the subclass definition.
I am not fully sure if that is technically feasible, as the mocks actually redirect calls to a call log, to be able to capture invocations and react/assert on them for testing purposes.
I'll investigate some more before making a call on that.

@sjrd
Copy link

sjrd commented Jul 9, 2017

What that means is that means the macros will have to inspect the tree of the type-to-mock and check if any parameters and/or implementation are defined as js.native

You don't have to inspect the trees for that. The body of a concrete method A.m will be = js.native if and only if A is annotated with @js.native.

@barkhorn
Copy link
Collaborator

barkhorn commented Jul 9, 2017

good to know, but still need to check parameters for the defaults (which need repeating)?

@sjrd
Copy link

sjrd commented Jul 9, 2017

For the defaults, if they are in an @js.native class/trait, they need to be repeated regardless of their body (be it = js.native or anything else). The problem is that there is no good way to automatically repeat them, because their value is only relevant for documentation, as far as the compiler is concerned. For example, a lot of default values are = ??? or js.native, which is totally nonsensical if interpreted as actual code (as they would be in a non-native trait/class), but is fine in a native JS type because it's ignored by the compiler.

@wgillett
Copy link
Author

Noting for the record that I tried to implement the workaround discussed earlier, turns out to be not so easy. The idea proposed by @barkhorn (see above) is to create an adaptor trait CanvasAdaptor implementing all the CanvasRenderingContext2D methods, then an implementation class class RealCanvas extends CanvasRenderingContext2D with CanvasAdaptor. Code all methods to take an arg of type CanvasAdaptor, then it's easy to pass in mocks.

The first problem is that we get a canvas in the first place by code like this:
dom.window.document.getElementById(id) .asInstanceOf[html.Canvas] .getContext("2d") .asInstanceOf[dom.CanvasRenderingContext2D]

Now we have our hands on a CanvasRenderingContext2D, which doesn't implement the CanvasAdaptor trait. I think the answer is to have RealCanvas instead wrap an instance of CanvasRenderingContext2D: class RealCanvas(_canvas: CanvasRenderingContext2D) extends CanvasAdaptor. So after making the CanvasRenderingContext2D instance (see above), we call new RealCanvas(canvas) to do the wrapping. The RealCanvas class has to be coded to forward references to its 55 vars and methods to the contained DOM canvas, which is tedious but doable (although this extra call adds some runtime overhead in the non-testing context).

Next problem is that methods like this with js.native default arg values: def fillText(text: String, x: Double, y: Double, maxWidth: Double = js.native): Unit = js.native cannot simply be copied to the CanvasAdaptor trait, because only the framework is allowed to use js.native. So in the interface we have to drop all the js.native arg defaults and rely on overloading instead to provide defaults. For example, define a variant def fillText(text: String, x: Double, y: Double): Unit = js.native that omits the maxWidth arg.

Having done all that, the resulting code sort of kind of works, but there is some strange runtime behavior (e.g., getting a font string with an invalid size: 14.14.4px sans-serif) that I can't figure out. Likely I'm making some Scala.js noobie mistakes in the implementation, but for now I need to put this on hold in order to make progress. Thanks for everyone's efforts to figure out how to get the framework to handle this case properly!

@barkhorn
Copy link
Collaborator

barkhorn commented Aug 5, 2017

added a test case at https://github.com/paulbutcher/ScalaMock/blob/master/js/src/test/scala/org/scalamock/jstests/JSNativeTest.scala but wasn't able to develop a fix yet.

@barkhorn barkhorn added this to the v4.0.0 milestone Sep 25, 2017
@barkhorn barkhorn modified the milestones: v4.0.0, v4.1.0 Oct 29, 2017
@barkhorn barkhorn modified the milestones: v4.1.0, v4.2.0 Feb 17, 2018
@barkhorn barkhorn modified the milestones: v4.2.0, v4.3.0 Apr 21, 2019
@barkhorn barkhorn removed the ready label Apr 21, 2019
@barkhorn barkhorn removed this from the v4.3.0 milestone Jun 23, 2019
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

3 participants