return 404 for route not found #35
Conversation
| ws.url("/url2").get() onComplete { | ||
| case Success(x)=> x.status should be(404) | ||
| case Failure(x)=> fail("should not throw an exception for url not found") | ||
| } |
There was a problem hiding this comment.
You can use await to directly assert the result of the Future:
await(ws.url("/url2").get()).status should be (404)
There was a problem hiding this comment.
SOrry, didn't pay attention, it's not Async test.
| //If we think of it as exception with failed future then it will suppress the semantics of | ||
| //client failure vs HTTP failures. Caller should check for HTTP status rather than future state | ||
| logger.debug(s"no route defined for $method $url") | ||
| Future.successful(Results.NotFound) |
|
What is your opinion about what to do when no route can be found? |
|
Perhaps the best way I can think of is if the function can take an implicit parameter |
|
@sahilahmadlone that's a good idea. And we can provide a good default (like returning a 404) |
|
Will work on it and let you know. |
|
@yanns Can you have a look at it? |
src/main/scala/mockws/MockWS.scala
Outdated
| @@ -59,5 +61,19 @@ object MockWS { | |||
| * @param routes simulation of the external web resource | |||
| * @param materializer user-defined materializer | |||
| */ | |||
| def apply(routes: Routes, materializer: ActorMaterializer) = new MockWS(routes, () ⇒ Unit)(materializer) | |||
| def apply(routes: Routes, materializer: ActorMaterializer)(implicit fn: RouteNotDefined) = { | |||
There was a problem hiding this comment.
I'd prefer an apply without any implicit parameters:
def apply(routes: Routes, materializer: ActorMaterializer, notFoundBehaviour: RouteNotDefined) =
new MockWS(routes, () ⇒ Unit)(materializer, notFoundBehaviour)
There was a problem hiding this comment.
If we do that , then in this case it will be picked from implicit scope
There was a problem hiding this comment.
I think what we can do is have only one apply and we put the defaults in implicit scope and the user can override if he wants to , what do you think ?
There was a problem hiding this comment.
actually, by thinking again about it, you can let this code as it is now.
src/main/scala/mockws/MockWS.scala
Outdated
| @@ -31,7 +33,7 @@ import play.api.mvc.EssentialAction | |||
| * | |||
| * @param routes routes defining the mock calls | |||
| */ | |||
| class MockWS(routes: MockWS.Routes, shutdownHook: () ⇒ Unit)(implicit val materializer: ActorMaterializer) extends WSClient { | |||
| class MockWS(routes: MockWS.Routes, shutdownHook: () ⇒ Unit)(implicit val materializer: ActorMaterializer, action: RouteNotDefined) extends WSClient { | |||
There was a problem hiding this comment.
I like the name notFoundBehaviour you used before.
Could you use it here instead of action?
src/main/scala/mockws/MockWS.scala
Outdated
| trait RouteNotDefined extends (() => Future[Result]) | ||
| object RouteNotDefined { | ||
|
|
||
| implicit val defaultAction:RouteNotDefined = RouteNotDefined(NotFound) |
There was a problem hiding this comment.
Nitpick: add a whitespace between defaultAction: and RouteNotDefined
There was a problem hiding this comment.
We can try to add a scalafmt plugin , maybe a new PR after this
src/main/scala/mockws/MockWS.scala
Outdated
| @@ -49,7 +51,7 @@ object MockWS { | |||
| /** | |||
| * @param routes simulation of the external web resource | |||
| */ | |||
| def apply(routes: Routes) = { | |||
| def apply(routes: Routes)(implicit fn: RouteNotDefined) = { | |||
There was a problem hiding this comment.
same here, please use notFoundBehaviour.
|
If you have any other project you need help with, give me a shout. |
If there is no route, that means the future is successful but the server returned 404
If we think of it as an exception with failed future then it will suppress the semantics of
client failure vs HTTP failures. Caller should check for HTTP status rather than future state