Skip to content

Support multiple matchers in MockMvc Kotlin DSL #24103

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
devtribe opened this issue Nov 29, 2019 · 6 comments
Closed

Support multiple matchers in MockMvc Kotlin DSL #24103

devtribe opened this issue Nov 29, 2019 · 6 comments
Assignees
Labels
in: test Issues in the test module status: feedback-provided Feedback has been provided type: bug A general bug
Milestone

Comments

@devtribe
Copy link

devtribe commented Nov 29, 2019

Affects: 5.2.1.RELEASE

I would expect that in a DSL everything in a lambda block will be executed.
But if i try to assert multiple matchers in the model block only the last one will be executed.
I didn't get any feedback about this behaviour, so i get the false impression that my code is executed, if it isn't.

Doesn't work like expected:

mockMvc.get(url)
  .andExpect {
    model {
      attribute("foo", "foo")
      attribute("bar", "bar")
    }
}

Works, but is redundant code:

mockMvc.get(url)
  .andExpect {
    model { attribute("foo", "foo") }
    model { attribute("bar", "bar") }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 29, 2019
@sdeleuze sdeleuze self-assigned this Nov 29, 2019
@sbrannen sbrannen added the in: test Issues in the test module label Nov 29, 2019
@sdeleuze sdeleuze added this to the 5.2.3 milestone Nov 29, 2019
@sdeleuze sdeleuze added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 29, 2019
@sdeleuze
Copy link
Contributor

I can understand how it could be confusing, but here we are reusing the ModelResultMatchers Java API:

fun model(matcher: ModelResultMatchers.() -> ResultMatcher) {
	actions.andExpect(MockMvcResultMatchers.model().matcher())
}

One way of supporting your use case would be to create a ModelResultMatchersDsl that would support multiple matchers.

Any thoughts @checketts @jnizet?

@sdeleuze sdeleuze added the status: waiting-for-feedback We need additional information before we can continue label Dec 2, 2019
@checketts
Copy link

I absolutely want a ModelResultMatchersDsl (and would even be interested in implementing it).

I'll be converting my tests over from the kd4smt to this new DSL and I suspect I'll run into these sorts of issues.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 3, 2019
@sdeleuze
Copy link
Contributor

@checketts Do you want to provide a PR?
@jnizet Any thoughts?

@sdeleuze sdeleuze modified the milestones: 5.2.3, 5.2.4 Jan 13, 2020
@jnizet
Copy link

jnizet commented Jan 13, 2020

@sdeleuze I haven't been using the DSL in a while, and I've never been hit by this issue myself but I agree that the current situation is confusing, and that a ModelResultMatchersDsl would be nice.

@jhoeller jhoeller modified the milestones: 5.2.4, 5.2.5 Feb 20, 2020
@bclozel bclozel modified the milestones: 5.2.5, 5.2.6 Mar 16, 2020
@sdeleuze sdeleuze modified the milestones: 5.2.6, 5.3 M1 Apr 13, 2020
@sdeleuze sdeleuze modified the milestones: 5.3 M1, 5.3 M2 Jun 8, 2020
@jhoeller jhoeller modified the milestones: 5.3 M2, 5.3 RC1 Aug 7, 2020
@sdeleuze sdeleuze modified the milestones: 5.3 RC1, 5.3 RC2 Aug 26, 2020
@sdeleuze sdeleuze modified the milestones: 5.3 RC2, 5.3 GA Sep 25, 2020
@sdeleuze sdeleuze changed the title MockMvc Kotlin DSL should work with multiple ResultMatcher under model{} MockMvc Kotlin DSL should support multiple ResultMatchers Oct 26, 2020
@sdeleuze sdeleuze changed the title MockMvc Kotlin DSL should support multiple ResultMatchers Support multiple matchers in MockMvc Kotlin DSL Oct 26, 2020
@sdeleuze
Copy link
Contributor

@devtribe Thanks for raising this important issue, I think it is now properly fixed.

@LifeIsStrange
Copy link

LifeIsStrange commented Nov 22, 2020

@sdeleuze while this seems like an important fix, I really miss the property access syntax of isOk, etc.
is'nt there a way to undo this breaking change just for the status { } block?
From my understanding (I might be wrong) the status block is always intended to only have only one statement, therefore being unaffected by this issue, but still, the fix has a side effect on this block. could the previous way be used specifically for the status block so that we could have the best of both worlds?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
in: test Issues in the test module status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

No branches or pull requests

9 participants