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

Update to MockHttpServletRequest and its test #1516

Merged
merged 6 commits into from Mar 16, 2014

Conversation

Projects
None yet
4 participants
@dmclean62
Copy link
Contributor

dmclean62 commented Jan 23, 2014

Fix for problem where a parameter in the URL has a blank value (either "a=" or just "a").

dmclean62 added some commits Jan 23, 2014

Update MockHttpRequestSpec.scala
Added test for parameters with empty values
Update MockHttpServletRequest.scala
Add case matching a parameter with no/empty value ("a=" or just "a")
@fmpwizard

This comment has been minimized.

Copy link
Member

fmpwizard commented Jan 23, 2014

Looks good, we just need one small change, to add your name and email address to the https://github.com/lift/framework/blob/master/contributors.md file

@dmclean62

This comment has been minimized.

Copy link
Contributor Author

dmclean62 commented Jan 23, 2014

Like that?

On Thu, Jan 23, 2014 at 1:31 PM, Diego Medina notifications@github.comwrote:

Looks good, we just need one small change, to add your name and email
address to the
https://github.com/lift/framework/blob/master/contributors.md file


Reply to this email directly or view it on GitHubhttps://github.com//pull/1516#issuecomment-33153383
.

Family photographs are a critical legacy for
ourselves and our descendants. Protect that
legacy with a digital backup and recovery plan.

@fmpwizard

This comment has been minimized.

Copy link
Member

fmpwizard commented Jan 23, 2014

👍

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Jan 24, 2014

Hm. How do I run the MockHttpRequestSpec?

@dmclean62

This comment has been minimized.

Copy link
Contributor Author

dmclean62 commented Jan 24, 2014

Actually, I have no idea. I don't have a Lift development environment and I
tested the change in another way, but it seemed like the Lift test should
also be updated. I made a copy of an existing test with only the most minor
changes to reflect what was needed, but I did it entirely on the Github web
site.

On Fri, Jan 24, 2014 at 2:04 PM, Antonio Salazar Cardozo <
notifications@github.com> wrote:

Hm. How do I run the MockHttpRequestSpec?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1516#issuecomment-33251172
.

@fmpwizard

This comment has been minimized.

Copy link
Member

fmpwizard commented Jan 24, 2014

this should do:

> test-only net.liftweb.mocks.MockHttpRequestSpec
@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Jan 24, 2014

Derp. Heh. I was in the wrong project.

Anyway, the test isn't passing, and broke another one ;)

[error] x parse parameters with empty values
[error]    'https://foo.com/test/this/page?a=b&b=a&c=&d='
[error]     is not equal to
[error]    'https://foo.com/test/this/page?a=b&b=a&a=c' (MockHttpRequestSpec.scala:58)
[error] Expected: ...&b=a&[a]=[c]
[error] Actual:   ...&b=a&[c]=[&d=]
...
[error] x throw an IllegalArgumentException for an invalid query string
[error]    Expected: java.lang.IllegalArgumentException. Got nothing (MockHttpRequestSpec.scala:90)
@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Jan 24, 2014

So it looks like a couple of things need to happen:

  • case Array(key) is not valid if key.isEmpty, so we need a guard on that match
  • In the test, you need to compare getRequestURL.toString to the appropriate expected URL
  • In the test, you need to compare getQueryString to the appropriate expected query string
@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Feb 2, 2014

Any updates on this?

@dmclean62

This comment has been minimized.

Copy link
Contributor Author

dmclean62 commented Feb 2, 2014

I apologize for being distracted. Our FIOS was out for several days, and
since then things have been very busy. I'll look at this tomorrow morning.

On Sun, Feb 2, 2014 at 10:32 AM, Antonio Salazar Cardozo <
notifications@github.com> wrote:

Any updates on this?

Reply to this email directly or view it on GitHubhttps://github.com//pull/1516#issuecomment-33902984
.

Family photographs are a critical legacy for
ourselves and our descendants. Protect that
legacy with a digital backup and recovery plan.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Feb 2, 2014

No problem, just wanted to ping and see :)

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Feb 7, 2014

Let me know if you need someone else to step in and make the remaining changes; I may have a moment to look at it this weekend and wrap things up :)

@dmclean62

This comment has been minimized.

Copy link
Contributor Author

dmclean62 commented Feb 7, 2014

a. Got it - there's an actual test for two consecutive ampersands.
b. duh
c. duh

I can't believe I made such silly noob mistakes. Sorry about that.

On Fri, Jan 24, 2014 at 3:35 PM, Antonio Salazar Cardozo <
notifications@github.com> wrote:

So it looks like a couple of things need to happen:

  • case Array(key) is not valid if key.isEmpty, so we need a guard on
    that match
  • In the test, you need to compare getRequestURL.toString to the
    appropriate expected URL
  • In the test, you need to compare getQueryString to the appropriate
    expected query string
@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Feb 7, 2014

Ok, looks like that double-ampersand test also specifically tests the situation where the querystring is just foo, which was an IllegalArgumentException but shouldn't be anymore. So line 91 in the spec should probably go away, or change to must not throwA.... So that test is still failing at the moment…

The other test also still isn't passing, because you're expecting the serialization of the no-value parameter to just be d, but it'll be d=. Changing lines 58 and 59 of the spec to add an = to the end of the expected strings should fix that :)

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Feb 7, 2014

(As a side note, to double-check the test you can just:

$ sbt
> project lift-testkit
> test

)

@farmdawgnation

This comment has been minimized.

Copy link
Member

farmdawgnation commented Feb 15, 2014

It'd be really cool to get this into M3. @dmclean62 Anything I can do to help out? :)

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Mar 16, 2014

@dmclean62 I'm going to go ahead and apply the last couple of fixes and merge this in today I think :)

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