Skip to content

Polish tests #436

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

Closed
wants to merge 1 commit into from
Closed

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Oct 12, 2017

This PR polishes some tests.

@wilkinsona wilkinsona added this to the 1.2.3.RELEASE milestone Oct 13, 2017
@wilkinsona wilkinsona added the type: task Non user-facing work label Oct 13, 2017
@wilkinsona
Copy link
Member

Thanks, @izeye, but I don't think we can merge these changes.

The changes to JsonContentHandlerTests aren't necessary as the intention is that the field type is wrong.

The changes to JsonFieldProcessorTests don't build cleanly with 1.8.0_141 (Oracle Corporation 25.141-b15). For example:

/Users/awilkinson/dev/spring/spring-restdocs/spring-restdocs-core/src/test/java/org/springframework/restdocs/payload/JsonFieldProcessorTests.java:52: error: no suitable method found for assertThat(Object,Matcher<String>)
                assertThat(this.fieldProcessor.extract("a", payload).getValue(),
                ^
    method Assert.<T#1>assertThat(String,T#1,Matcher<? super T#1>) is not applicable
      (cannot infer type-variable(s) T#1
        (actual and formal argument lists differ in length))
    method Assert.<T#2>assertThat(T#2,Matcher<? super T#2>) is not applicable
      (inferred type does not conform to upper bound(s)
        inferred: Object
        upper bound(s): String,Object)
  where T#1,T#2 are type-variables:
    T#1 extends Object declared in method <T#1>assertThat(String,T#1,Matcher<? super T#1>)
    T#2 extends Object declared in method <T#2>assertThat(T#2,Matcher<? super T#2>)

@wilkinsona wilkinsona closed this Oct 13, 2017
@wilkinsona wilkinsona removed this from the 1.2.3.RELEASE milestone Oct 13, 2017
@wilkinsona wilkinsona added the status: declined Suggestion or change that we don't want to make at this time label Oct 13, 2017
@izeye
Copy link
Contributor Author

izeye commented Oct 14, 2017

@wilkinsona Thanks for the feedback!

The changes to JsonContentHandlerTests aren't necessary as the intention is that the field type is wrong.

I thought the intention for either JsonContentHandlerTests.typeForFieldWithNotNullAndThenNullValueMustMatch() or JsonContentHandlerTests.typeForFieldWithNullAndThenNotNullValueMustMatch() was testing type mismatch between a specific type (NUMBER) and null which are VARIES and the specific type (NUMBER) unless it's not optional() but based on your comment, my assumption was wrong.

The changes to JsonFieldProcessorTests don't build cleanly with 1.8.0_141 (Oracle Corporation 25.141-b15).

I tried with the same version of JDK as yours but failed to see the build error:

Johnnyui-MacBook-Pro:spring-restdocs izeye$ ./gradlew -v

------------------------------------------------------------
Gradle 3.4.1
------------------------------------------------------------

Build time:   2017-03-03 19:45:41 UTC
Revision:     9eb76efdd3d034dc506c719dac2955efb5ff9a93

Groovy:       2.4.7
Ant:          Apache Ant(TM) version 1.9.6 compiled on June 29 2015
JVM:          1.8.0_141 (Oracle Corporation 25.141-b15)
OS:           Mac OS X 10.12.6 x86_64

Johnnyui-MacBook-Pro:spring-restdocs izeye$ ./gradlew clean check
:buildSrc:compileJava NO-SOURCE
:buildSrc:compileGroovy UP-TO-DATE
:buildSrc:processResources UP-TO-DATE
:buildSrc:classes UP-TO-DATE
:buildSrc:jar UP-TO-DATE
:buildSrc:assemble UP-TO-DATE
:buildSrc:compileTestJava NO-SOURCE
:buildSrc:compileTestGroovy NO-SOURCE
:buildSrc:processTestResources NO-SOURCE
:buildSrc:testClasses UP-TO-DATE
:buildSrc:test NO-SOURCE
:buildSrc:check UP-TO-DATE
:buildSrc:build UP-TO-DATE
:docs:clean
:spring-restdocs-asciidoctor:clean
:spring-restdocs-core:clean
:spring-restdocs-mockmvc:clean
:spring-restdocs-restassured:clean
:docs:compileJava NO-SOURCE
:docs:processResources NO-SOURCE
:docs:classes UP-TO-DATE
:spring-restdocs-core:jmustacheRepackJar
:spring-restdocs-core:compileJava
:spring-restdocs-core:processResources
:spring-restdocs-core:classes
:spring-restdocs-core:jar
:spring-restdocs-mockmvc:compileJava
:spring-restdocs-mockmvc:processResources NO-SOURCE
:spring-restdocs-mockmvc:classes
:spring-restdocs-mockmvc:jar
:spring-restdocs-restassured:compileJava
:spring-restdocs-restassured:processResources NO-SOURCE
:spring-restdocs-restassured:classes
:spring-restdocs-restassured:jar
:docs:compileTestJava
:docs:processTestResources NO-SOURCE
:docs:testClasses
:docs:test
:docs:check
:spring-restdocs-asciidoctor:compileJava
:spring-restdocs-asciidoctor:processResources
:spring-restdocs-asciidoctor:classes
:spring-restdocs-asciidoctor:checkstyleMain
:spring-restdocs-asciidoctor:compileTestJava
:spring-restdocs-asciidoctor:processTestResources
:spring-restdocs-asciidoctor:testClasses
:spring-restdocs-asciidoctor:checkstyleTest
:spring-restdocs-asciidoctor:test
:spring-restdocs-asciidoctor:check
:spring-restdocs-core:checkstyleMain
:spring-restdocs-core:compileTestJava
:spring-restdocs-core:processTestResources
:spring-restdocs-core:testClasses
:spring-restdocs-core:checkstyleTest
:spring-restdocs-core:test
objc[31631]: Class JavaLaunchHelper is implemented in both /Library/Java/JavaVirtualMachines/jdk1.8.0_141.jdk/Contents/Home/bin/java (0x1094674c0) and /Library/Java/JavaVirtualMachines/jdk1.8.0_141.jdk/Contents/Home/jre/lib/libinstrument.dylib (0x10a4d64e0). One of the two will be used. Which one is undefined.
:spring-restdocs-core:check
:spring-restdocs-mockmvc:checkstyleMain
:spring-restdocs-core:testJar
:spring-restdocs-mockmvc:compileTestJava
:spring-restdocs-mockmvc:processTestResources
:spring-restdocs-mockmvc:testClasses
:spring-restdocs-mockmvc:checkstyleTest
:spring-restdocs-mockmvc:test
objc[31634]: Class JavaLaunchHelper is implemented in both /Library/Java/JavaVirtualMachines/jdk1.8.0_141.jdk/Contents/Home/bin/java (0x10cd714c0) and /Library/Java/JavaVirtualMachines/jdk1.8.0_141.jdk/Contents/Home/jre/lib/libinstrument.dylib (0x10edf54e0). One of the two will be used. Which one is undefined.
:spring-restdocs-mockmvc:check
:spring-restdocs-restassured:checkstyleMain
:spring-restdocs-restassured:compileTestJava
:spring-restdocs-restassured:processTestResources
:spring-restdocs-restassured:testClasses
:spring-restdocs-restassured:checkstyleTest
:spring-restdocs-restassured:test
objc[31637]: Class JavaLaunchHelper is implemented in both /Library/Java/JavaVirtualMachines/jdk1.8.0_141.jdk/Contents/Home/bin/java (0x10c93f4c0) and /Library/Java/JavaVirtualMachines/jdk1.8.0_141.jdk/Contents/Home/jre/lib/libinstrument.dylib (0x10d9ae4e0). One of the two will be used. Which one is undefined.
:spring-restdocs-restassured:check

BUILD SUCCESSFUL

Total time: 1 mins 14.389 secs
Johnnyui-MacBook-Pro:spring-restdocs izeye$

How did I see the build error?

@wilkinsona
Copy link
Member

but based on your comment, my assumption was wrong.

It was really just checking that a failure occurs when the type doesn't match so anything other than VARIES is fine. What is missing at this level, though, is a test that checks that the types is determined to VARIES rather than just something other than what the test expects. I've opened #437.

I tried with the same version of JDK as yours but failed to see the build error:

Well this is very strange indeed. It only happens when I cherry-pick the change back onto the 1.2.x branch. I'd guess it's a limitation of the type inferencing in Java 8's javac when source or target compatibility is set to 1.7. The means that change could be applied only to master but I think I'd prefer not to do so.

@izeye
Copy link
Contributor Author

izeye commented Oct 16, 2017

@wilkinsona #437 looks already covered in https://github.com/spring-projects/spring-restdocs/blob/master/spring-restdocs-core/src/test/java/org/springframework/restdocs/payload/JsonContentHandlerTests.java#L79-L93, right?

JsonFieldProcessorTests polishing has two parts. The first part is removing Object casting which has been declined and the second part is removing .getValue() invocation.

I think the second part, removing .getValue() invocation makes the intention of tests clear as the expected exception, FieldDoesNotExistException will be thrown in .extract() invocation, not .getValue() invocation. WDYT?

@wilkinsona
Copy link
Member

Thanks, I've closed #437.

Removing getValue() makes sense. Let me grab that bit and merge it. Thanks for the nudge.

@wilkinsona wilkinsona reopened this Oct 16, 2017
@wilkinsona wilkinsona removed the status: declined Suggestion or change that we don't want to make at this time label Oct 16, 2017
@wilkinsona wilkinsona added this to the 1.2.3.RELEASE milestone Oct 16, 2017
@izeye izeye closed this Oct 16, 2017
@izeye izeye force-pushed the polish-tests-20171013 branch from dfa2a78 to 3d7262a Compare October 16, 2017 16:33
@wilkinsona
Copy link
Member

@izeye Let's keep this open. I can cherry-pick the bits that we want.

@wilkinsona wilkinsona reopened this Oct 16, 2017
@izeye
Copy link
Contributor Author

izeye commented Oct 16, 2017

@wilkinsona Sorry, I didn't mean to close this PR. I accidentally force-pushed empty commit and that closed this PR 😅

wilkinsona added a commit that referenced this pull request Oct 20, 2017
@wilkinsona
Copy link
Member

Thanks once again, @izeye. This has been merged into 1.2.x and forward into master.

@izeye izeye deleted the polish-tests-20171013 branch October 20, 2017 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task Non user-facing work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants