Warn when using shouldBe between different types

[Kantis]

[sksamuel]

Is this ready for review ?

[Kantis]

Conceptually, sure. I think I'd like to build and run the plugin with the changes for a while before claiming it's good to go for real tho. Just haven't gotten around to it yet.. 🙈

[sksamuel]

Ok makes sense.

On Mon, 22 Nov 2021 at 15:56, Emil Kantis @.***> wrote:

Conceptually, sure. I think I'd like to build and run the plugin with the changes for a while before claiming it's good to go for real tho. Just haven't gotten around to it yet.. 🙈

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-975952906, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGTYNQBKODC27GXLXFLUNK37DANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

Managed to install it and played around with it for a bit.. Seems like it can be really useful to me 😄

I also tried to make sure it didn't break performance. I opened some of the heavier files in the kotest and it didn't feel unusually slow to me. Tested with TagsTest and StringMatchersTest, since they to p usage of shouldBe, and DoubleMatchersTest since it's just big in general..

[sksamuel]

Is it ready for review and test ?

On Wed, 24 Nov 2021, 16:38 Emil Kantis, @.***> wrote:

Managed to install it and played around with it for a bit.. Seems like it can be really useful to me 😄

I also tried to make sure it didn't break performance. I opened some of the heavier files in the kotest and it didn't feel unusually slow to me. Tested with TagsTest and StringMatchersTest, since they to p usage of shouldBe, and DoubleMatchersTest since it's just big in general..

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-978373170, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGU3QIEOY2X6N5LR3RLUNVSPHANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

I'm sorry, I discovered a couple of problems last night. I ran the inspection on Kotest and Hoplite sources and learnt that shouldBe supports matching T with Matcher<T>. Also, any pair of types that have equals implementations would give warnings, even though the assertion would pass. Would be easy to not give any warnings when either type is Matcher, but not sure how to deal with the 2nd case..

[sksamuel]

What about keeping it simple. If RHS is Matcher, don't run inspection If RHS is a primitive or String, then the types must match. Otherwise, LHS <:< RHS or RHS <:< LHS where <:< means subtype or type

[Kantis]

Thanks for the input, makes sense..

Fixed the Matcher handling (by simply checking if RHS is a type named *Matcher*)

Inspection currently passes for primitives which I think kotest handles comparison of gracefully (Int and Long, Double and Float, along with their respective primitive arrays). Should I ditch that? If not - this is ready for final review and test.

BTW - I don't get why the build is failing for 212 only.. I've been running the plugin on 212 for a while.. 🤯

[sksamuel]

I don't think ints and longs compare in kotest.

On Wed, 1 Dec 2021 at 18:40, Emil Kantis @.***> wrote:

Thanks for the input, makes sense.. Fixed the Matcher handling (by simply checking if RHS is a type named Matcher) Inspection currently passes for primitives which I think kotest handles comparison of gracefully (Int and Long, Double and Float, along with their respective primitive arrays). Should I ditch that?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-984186589, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGR5D7T7S45RH2ZPSNTUO26BPANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

@sksamuel this test passes for me, so it feels like they do? Am I missing something?

      test("dummy") {
         val i: Int = 6
         i shouldBe 6L
      }

[sksamuel]

I thought it failed when I tried. What about 6L shouldBe 2 ?

On Wed, 1 Dec 2021 at 22:48, Emil Kantis @.***> wrote:

@sksamuel https://github.com/sksamuel this test passes for me, so it feels like they do? Am I missing something?

  test("dummy") {
     val i: Int = 6
     i shouldBe 6L
  }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-984289767, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGUPAFKBXHQGWBR77WTUO33B5ANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

      test("dummy 1") {
         6L shouldBe 2
      }

-->
expected:<2L> but was:<6L>
Expected :2L
Actual   :6L



      test("dummy 2") {
         val y: Int = 2
         6L shouldBe y
      }
-->
expected:<2> but was:<6L>
Expected :2
Actual   :6L

[sksamuel]

There you go.

On Wed, 1 Dec 2021 at 23:18, Emil Kantis @.***> wrote:

  test("dummy 1") {
     6L shouldBe 2
  }

--> expected:<2L> but was:<6L> Expected :2L Actual :6L

  test("dummy 2") {
     val y: Int = 2
     6L shouldBe y
  }

--> expected:<2> but was:<6L> Expected :2 Actual :6L

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-984301814, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGUOYLA3ZXFKBZ7O6H3UO36RHANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

I dont get it.. It's failing when we have different values, but passing when the same number? Feels like it should be considered fine then to compare ints and longs?

[sksamuel]

Oh wait, it's always going to fail on different values LOL.

Yeah you're probably right,

What about 6L shouldBe 6 ?

On Wed, 1 Dec 2021 at 23:25, Emil Kantis @.***> wrote:

I dont get it.. It's failing when we have different values, but passing when the same number? Feels like it should be considered fine then to compare ints and longs?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-984304860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGS3JPOGCQAHGTYAEITUO37LFANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

      test("dummy 1") {
         6L shouldBe 6
      } --> pass

      test("dummy 2") {
         val y: Int = 6
         6L shouldBe y
      } --> pass

[sksamuel]

Ok then I guess it's fine to say int/long compare.

On Wed, 1 Dec 2021 at 23:32, Emil Kantis @.***> wrote:

  test("dummy 1") {
     6L shouldBe 6
  } --> pass

  test("dummy 2") {
     val y: Int = 6
     6L shouldBe y
  } --> pass

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-984307674, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGS7XNGDACLGW24AHIDUO4AG3ANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

Awesome :) do you have any hunch as to why the tests fail on just IC-212? 🤔

[sksamuel]

Nope, perhaps it's just not setup right, that was a recent addition to the build matrix.

On Thu, 2 Dec 2021 at 02:41, Emil Kantis @.***> wrote:

Awesome :) do you have any hunch as to why the tests fail on just IC-212? 🤔

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kotest/kotest-intellij-plugin/pull/146#issuecomment-984410513, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGT3SE3I4RORXHQ5VPDUO4WKRANCNFSM5E6MK3ZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[Kantis]

Nope, perhaps it's just not setup right, that was a recent addition to the build matrix.

Doh, so I just had to update those plugin.xml files 😂 sorry. Should be ok now.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Kantis]

@sksamuel thx for re-opening.

I haven't gotten much feedback on this one so I'm still unsure if it has acceptable performance impact. I sometimes felt IntelliJ got a bit sluggish in large files, but not sure if it was due to these changes or just IntelliJ struggling. :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.