ScalaMock icon indicating copy to clipboard operation
ScalaMock copied to clipboard

error when mocking a Scala.js native method

Open wgillett opened this issue 7 years ago • 16 comments

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]
    }
  }
}

wgillett avatar Jul 06 '17 20:07 wgillett

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.

barkhorn avatar Jul 07 '17 08:07 barkhorn

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

wgillett avatar Jul 07 '17 15:07 wgillett

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

barkhorn avatar Jul 08 '17 08:07 barkhorn

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 avatar Jul 08 '17 09:07 barkhorn

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

barkhorn avatar Jul 08 '17 09:07 barkhorn

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.

wgillett avatar Jul 08 '17 17:07 wgillett

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 avatar Jul 09 '17 08:07 sjrd

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

sjrd avatar Jul 09 '17 09:07 sjrd

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?

barkhorn avatar Jul 09 '17 11:07 barkhorn

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

sjrd avatar Jul 09 '17 12:07 sjrd

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.

barkhorn avatar Jul 09 '17 12:07 barkhorn

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.

sjrd avatar Jul 09 '17 12:07 sjrd

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

barkhorn avatar Jul 09 '17 12:07 barkhorn

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.

sjrd avatar Jul 09 '17 13:07 sjrd

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!

wgillett avatar Jul 12 '17 22:07 wgillett

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 avatar Aug 05 '17 20:08 barkhorn