-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix assertTrue on new instance creation #9832
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
Conversation
bfa29b8
to
9a60206
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
object SmartAssertionScala3Spec extends ZIOBaseSpec { | ||
|
||
override def spec = | ||
suite("SmartAssertionScala3Spec")( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this only contain Scala 3 specific tests? Do we need the overlap with SmartAssertionSpec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, only specific to scala 3 test cases.
- trait with parameters
- inlines
- assertTrue with anonymous classes not compiled with scala 2 and it looked like scala 2 problem itself
If you have any suggestions on improving it - I can do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't think about traits with parameters not being available on Scala 2. Thanks for the clarification.
.find(f => f.name == name) | ||
.orElse(s.methodMember(name).filter(_.declarations.nonEmpty).headOption) | ||
.getOrElse( | ||
throw new Error(s"Could not resolve $name on ${owner.show(using Printer.TreeStructure)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use reportErrorAndAbort
? Is it also possible to test these failure cases, or do we expect them to be unreachable (for now)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll change it.
This should be unreachable if chosen TypeRepr
if correct, because we know here that was selected a val or a method without arguments, so it should exist. It can be reachable only in case of incorrect implementation.
This PR improves support for creation of new objects in
assertTrue
macro in case of overloaded methodsThere were several problems:
Block(List(cls: ClassDef), term)
and it has no symbol. So we catch it and take correct symbol.Typed(Block(List(cls: ClassDef), term), _)
and it has no symbol too. Solution is same, but also I usedunderlyingArgument
to removeInlined
nodes.Also:
getFieldOrMethod
to remove some boilerplate. Now it acceptsTypeRepr
and takes itstypeSymbol
. Note that it affects one of cases wherelhs.symbol
was used and now it islhs.tpe.typeSymbol
. IMO usinglhs.symbol
is not correct and I'm sure about it - if you think I'm wrong, we can discuss it.inline
.#9683