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

Class literals are not properly rendered (or rendered at all) #909

Open
Vampire opened this Issue Sep 13, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@Vampire
Contributor

Vampire commented Sep 13, 2018

Issue description

If you test classes with class literals, they are not rendered correctly, neither in the normal failure output, nor if you click on Click to see differences in IntelliJ IDEA.

How to reproduce

import spock.lang.Specification

class Test extends Specification {
    def 'both are rendered'() {
        expect:
            new SocketTimeoutException().getClass() == new ClassNotFoundException().getClass()
    }

    def 'only expected is rendered'() {
        expect:
            SocketTimeoutException == new ClassNotFoundException().getClass()
    }

    def 'only actual is rendered'() {
        expect:
            new SocketTimeoutException().getClass() == ClassNotFoundException
    }

    def 'none is rendered'() {
        expect:
            SocketTimeoutException == ClassNotFoundException
    }
}

Expected behavior

All works like in both are rendered.

Actual behavior

both are rendered

Condition not satisfied:

new SocketTimeoutException().getClass() == new ClassNotFoundException().getClass()
|                            |          |  |                            |
|                            |          |  |                            class java.lang.ClassNotFoundException
|                            |          |  java.lang.ClassNotFoundException
|                            |          false
|                            class java.net.SocketTimeoutException
java.net.SocketTimeoutException
 <Click to see difference>


	at Test.both are rendered(Test.groovy:6)

grafik

only expected is rendered

Condition not satisfied:

SocketTimeoutException == new ClassNotFoundException().getClass()
                       |  |                            |
                       |  |                            class java.lang.ClassNotFoundException
                       |  java.lang.ClassNotFoundException
                       false
 <Click to see difference>


	at Test.only expected is rendered(Test.groovy:11)

grafik

only actual is rendered

Condition not satisfied:

new SocketTimeoutException().getClass() == ClassNotFoundException
|                            |          |
|                            |          false
|                            class java.net.SocketTimeoutException
java.net.SocketTimeoutException
 <Click to see difference>


	at Test.only actual is rendered(Test.groovy:16)

grafik

none is rendered

Condition not satisfied:

SocketTimeoutException == ClassNotFoundException
                       |
                       false

Expected :
Actual   :
 <Click to see difference>


	at Test.none is rendered(Test.groovy:21)

grafik

Additional Environment information

Java/JDK

1.8.0_131

Groovy version

2.4.9

Build tool version

Gradle 4.8.1

Operating System

Windows 10

IDE

IntelliJ IDEA 2018.1.6

Build-tool dependencies used

testImplementation 'org.spockframework:spock-core:1.2-RC2-groovy-2.4'

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 13, 2018

Member

Hm, comparing the class attributes is not really helpful here. IMHO we should special case the handling of classes and just compare Class.getName(). Thoughts?

Member

leonard84 commented Sep 13, 2018

Hm, comparing the class attributes is not really helpful here. IMHO we should special case the handling of classes and just compare Class.getName(). Thoughts?

@Vampire

This comment has been minimized.

Show comment
Hide comment
@Vampire

Vampire Sep 13, 2018

Contributor

I guess that would be ok too.
But having nothing is not nice and having different things neither.
So however you do it, new ClassNotFoundException().getClass() and ClassNotFoundException should render the same.
And both in the text view and in the diff view. (I do not mean text view == diff view)
Whether you want to use the generic solution of a special-case here is up to you, I could live with both. :-)

Contributor

Vampire commented Sep 13, 2018

I guess that would be ok too.
But having nothing is not nice and having different things neither.
So however you do it, new ClassNotFoundException().getClass() and ClassNotFoundException should render the same.
And both in the text view and in the diff view. (I do not mean text view == diff view)
Whether you want to use the generic solution of a special-case here is up to you, I could live with both. :-)

@leonard84 leonard84 added the bug label Sep 13, 2018

@Vampire

This comment has been minimized.

Show comment
Hide comment
@Vampire

Vampire Sep 13, 2018

Contributor

Well, there is one use case where it is probably helpful.
When Foo == Foo returns false.
This can happen if they come from different class loaders.
(Assuming the class loader is displayed in the diff view, I cannot check right now, I'm on mobile)

Contributor

Vampire commented Sep 13, 2018

Well, there is one use case where it is probably helpful.
When Foo == Foo returns false.
This can happen if they come from different class loaders.
(Assuming the class loader is displayed in the diff view, I cannot check right now, I'm on mobile)

leonard84 added a commit that referenced this issue Sep 15, 2018

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 15, 2018

Member

It was quite easy to fix the rendering of the Class instances (see PR above). However, there are issues when trying to record ClassLiterals, e.g. instanceof expressions require a ClassLiterals. So I'm not going to do that for now. If anyone wants to give it a try org.spockframework.compiler.ConditionRewriter#visitClassExpression is the place to change.

Member

leonard84 commented Sep 15, 2018

It was quite easy to fix the rendering of the Class instances (see PR above). However, there are issues when trying to record ClassLiterals, e.g. instanceof expressions require a ClassLiterals. So I'm not going to do that for now. If anyone wants to give it a try org.spockframework.compiler.ConditionRewriter#visitClassExpression is the place to change.

leonard84 added a commit that referenced this issue Sep 16, 2018

@Vampire

This comment has been minimized.

Show comment
Hide comment
@Vampire

Vampire Sep 17, 2018

Contributor

I took a stab at this and fixed it. :-)

In my local code now:

  • Class instances and class literals are rendered in the ComparisonFailure now as <clazz.toString()> [<clazz.getClassLoader()>], so that it is also clear why Foo == Foo fails if the classes come from different class loaders
  • also class literals in instanceof expressions get the toString() rendered underneath in the failure text, so that you can easily see the FQDN (there was just a transform necessary from a instanceof Foo to Foo.class.isInstance(a))
  • it works with Foo, a.b.Foo, Foo.class and a.b.Foo.class
  • I found and fixed a bug where the recordCount got confused if there was a comment within or after a class expression that contained dots as the sourceText is used to "fix" the recordCount
  • added the possibility to record the expression results at runtime in arbitrary order (was necessary due to the transform from instanceof to isInstance() as there the evaluation order is different of course

What do you think about it?
I have to polish it a bit and maybe add a test or two, so I probably submit these as PRs only after my vacation, so second week of October or so.

Contributor

Vampire commented Sep 17, 2018

I took a stab at this and fixed it. :-)

In my local code now:

  • Class instances and class literals are rendered in the ComparisonFailure now as <clazz.toString()> [<clazz.getClassLoader()>], so that it is also clear why Foo == Foo fails if the classes come from different class loaders
  • also class literals in instanceof expressions get the toString() rendered underneath in the failure text, so that you can easily see the FQDN (there was just a transform necessary from a instanceof Foo to Foo.class.isInstance(a))
  • it works with Foo, a.b.Foo, Foo.class and a.b.Foo.class
  • I found and fixed a bug where the recordCount got confused if there was a comment within or after a class expression that contained dots as the sourceText is used to "fix" the recordCount
  • added the possibility to record the expression results at runtime in arbitrary order (was necessary due to the transform from instanceof to isInstance() as there the evaluation order is different of course

What do you think about it?
I have to polish it a bit and maybe add a test or two, so I probably submit these as PRs only after my vacation, so second week of October or so.

@marcphilipp

This comment has been minimized.

Show comment
Hide comment
@marcphilipp

marcphilipp Sep 17, 2018

Member

@leonard84 Shouldn't this be closed as it was fixed by #910?

Member

marcphilipp commented Sep 17, 2018

@leonard84 Shouldn't this be closed as it was fixed by #910?

@Vampire

This comment has been minimized.

Show comment
Hide comment
@Vampire

Vampire Sep 17, 2018

Contributor

#910 is only a partial fix, or rather no real fix at all. It changes how the class instances are rendered, but does not change at all what I describe here

Contributor

Vampire commented Sep 17, 2018

#910 is only a partial fix, or rather no real fix at all. It changes how the class instances are rendered, but does not change at all what I describe here

@Vampire

This comment has been minimized.

Show comment
Hide comment
@Vampire

Vampire Sep 17, 2018

Contributor

Btw. here you can have a look at the current state and how it changes existing test expectations. All pre-existing tests are green in this commit: Vampire@wip

Contributor

Vampire commented Sep 17, 2018

Btw. here you can have a look at the current state and how it changes existing test expectations. All pre-existing tests are green in this commit: Vampire@wip

@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Sep 17, 2018

Member

I added some inline comments, looks good so far. Maybe you also want to tackle the problem @szpak mentioned here #910 (comment)

Member

leonard84 commented Sep 17, 2018

I added some inline comments, looks good so far. Maybe you also want to tackle the problem @szpak mentioned here #910 (comment)

@Vampire

This comment has been minimized.

Show comment
Hide comment
@Vampire

Vampire Sep 17, 2018

Contributor

Thanks for having a look and confirming I'm on a good path. :-)

I pushed a new version that has the nulls gone and additionally I added a special case for Throwables in the expression info value renderer to also show the stacktrace.

Otherwise it is really hard to get helpful information e. g. if you have then: CompletionException ce = thrown(); with(ce.cause) { it; it.getClass() == NotFoundException }, and the cause is not what you expect. You only get the name and message. Now you also get the stacktrace. This really bugged me in the last days where I then always had to add it.printStackTrace() to get the info I needed. In the meantime I realised a nice workaround if you know that it is like that: then: CompletionException ce = thrown(); ce.cause; when: throw ce.cause; then: NotFoundException nfe = thrown()

Contributor

Vampire commented Sep 17, 2018

Thanks for having a look and confirming I'm on a good path. :-)

I pushed a new version that has the nulls gone and additionally I added a special case for Throwables in the expression info value renderer to also show the stacktrace.

Otherwise it is really hard to get helpful information e. g. if you have then: CompletionException ce = thrown(); with(ce.cause) { it; it.getClass() == NotFoundException }, and the cause is not what you expect. You only get the name and message. Now you also get the stacktrace. This really bugged me in the last days where I then always had to add it.printStackTrace() to get the info I needed. In the meantime I realised a nice workaround if you know that it is like that: then: CompletionException ce = thrown(); ce.cause; when: throw ce.cause; then: NotFoundException nfe = thrown()

Vampire referenced this issue in Vampire/spock Sep 18, 2018

@leonard84 leonard84 modified the milestones: 1.3, 1.1 Sep 29, 2018

MartyIX added a commit to MartyIX/spock that referenced this issue Oct 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment