truth icon indicating copy to clipboard operation
truth copied to clipboard

isInstanceOf/isNotInstanceOf can't find supertypes under GWT (if class metadata is off?)

Open cpovirk opened this issue 9 years ago • 4 comments

If I adopt isInstanceOf in Guava, various tests fail. These are tests that worked fine with assertTrue(foo instanceof Bar). Truth has special support for isInstanceOf under GWT, but it can't always work: My understanding is that getSuperclass() is unavailable if -XdisableClassMetadata is set:

https://code.google.com/p/google-web-toolkit/wiki/NoClassMetadataOptimization https://code.google.com/p/google-web-toolkit/source/detail?r=4790

There are various directions we could go here. We can fail isInstanceOf and/or isNotInstanceOf calls under GWT, whether unconditionally or only if class metadata is absent. We could fail some in one set of circumstances and some in the other. If we try to do our best without class metadata, we could add some warnings to the failure message. Or we can remove the methods entirely from GWT. (But that seems heavy handed, since I expect few users to run tests with class metadata off. Speaking of which, why not just turn on class metadata in Guava's tests? Our goal is to make sure that our prod code functions correctly with it off. This has the unfortunate effect of forcing our test code to do the same.)

I suggest something like this:

  • isNotInstanceOf fails under -XdisableClassMetadata, since we it can't do its job unless it's checking that the class is exactly == to another
  • isInstanceOf either does the same thing, or it tries to check == and then fails with a warning that it might be failing wrongly

(Also, we should expand our tests that these methods work correctly with class metadata on. We have some tests already, but I don't think we currently exercise isInstanceOf(SomeInterface.class).)

cpovirk avatar Aug 17 '15 16:08 cpovirk

A problem with my suggested plan: It works fine for assert/expect, but it does the wrong thing for assume, where the way of producing a falsely passing test is reversed.

We could try to special case assume, but that won't help with a custom FailureStrategy. My new suggestion would be for isInstanceOf to either fail always or to fail whenever the classes aren't ==. And by "fail," I mean "throw UnsupportedOperationException", not "call fail()".

cpovirk avatar Aug 19 '15 15:08 cpovirk

My 2 cents: these methods should be banned in GWT-compatible code until a better solution can be offered - isNotInstanceOf() will succeed erroneously, which is a serious problem.

dimo414 avatar Jan 03 '17 18:01 dimo414

You have shamed me into trying to do the easiest thing that might plug the biggest hole -- making isNotInstanceOf throw if class metadata is unavailable :) I'll report back.

cpovirk avatar Jan 03 '17 19:01 cpovirk

I forget the exact history here, but I think I left some things undone:

  • Run tests with class metadata off (for both j2cl and old-style GWT).
  • I think j2cl and old-style GWT may behave slightly differently. I thought I had left some notes on this somewhere, but I don't see them. Presumably the tests would find the problems again, at which point we should fix them.
  • We could still consider removing isInstanceOf and especially isNotInstanceOf entirely under GWT/j2cl, since they have significant limitations.

cpovirk avatar Aug 19 '19 15:08 cpovirk