New support for sequenced stub actions implemented. #49

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@shamoh
Contributor

shamoh commented May 6, 2016

Hi Mike.

This is suggestion how to extend Action API to support different response for identical requests repeated in sequence.

Let me know if you like the idea or if you prefer to make it different way.

Thanks,
Libor

shamoh added some commits May 6, 2016

New support for sequenced stub actions implemented.
Fixing guide links and formatting.
Updated version of Grizzly dependency to the latest v2.3.24.
@martinjmares

This comment has been minimized.

Show comment
Hide comment
@martinjmares

martinjmares May 9, 2016

I am supporting this idea! +1 for this pull request.
(I also like small fixes in documentation, which are included in the change.)

I am supporting this idea! +1 for this pull request.
(I also like small fixes in documentation, which are included in the change.)

@shamoh

This comment has been minimized.

Show comment
Hide comment
Contributor

shamoh commented May 9, 2016

+ .then().assertThat().body(equalTo("UPDATED VALUE"));
+ }
+
+}

This comment has been minimized.

@mkotsur

mkotsur May 9, 2016

Owner

I would like to add a test with at least two different conditions to make sure that the state is not thrown away when another condition kicks in.

@mkotsur

mkotsur May 9, 2016

Owner

I would like to add a test with at least two different conditions to make sure that the state is not thrown away when another condition kicks in.

This comment has been minimized.

@shamoh

shamoh May 9, 2016

Contributor

I have no problem to add another tests. But to be honest I don't know what you mean by "the state is not thrown away when another condition kicks in". Could describe little bit more verbose test steps? Thanks.

@shamoh

shamoh May 9, 2016

Contributor

I have no problem to add another tests. But to be honest I don't know what you mean by "the state is not thrown away when another condition kicks in". Could describe little bit more verbose test steps? Thanks.

This comment has been minimized.

@mkotsur

mkotsur May 10, 2016

Owner

Oh, I just mean:

Setup:

When /foo
Then OK200 and sequence STRINGfoo1, STRINfoo2

When /bar
Then OK200 and sequence STRINGbar1, STRINGbar2, STRINGbar3

Checks:

Get /foo expect STRINGfoo1
Get /bar expect STRINGbar1
Get /foo expect STRINGfoo2
...

And so on...

@mkotsur

mkotsur May 10, 2016

Owner

Oh, I just mean:

Setup:

When /foo
Then OK200 and sequence STRINGfoo1, STRINfoo2

When /bar
Then OK200 and sequence STRINGbar1, STRINGbar2, STRINGbar3

Checks:

Get /foo expect STRINGfoo1
Get /bar expect STRINGbar1
Get /foo expect STRINGfoo2
...

And so on...

This comment has been minimized.

@shamoh

shamoh May 13, 2016

Contributor

Done, I hope. ;)

@shamoh

shamoh May 13, 2016

Contributor

Done, I hope. ;)

This comment has been minimized.

@mkotsur

mkotsur May 14, 2016

Owner

Thanks, looks good. I think it is important to cover this case.

@mkotsur

mkotsur May 14, 2016

Owner

Thanks, looks good. I think it is important to cover this case.

@@ -293,6 +295,33 @@ public Response apply(Response input) {
}
+ /**
+ * Creates a sequence action which contains all passed actions and
+ * executes one by one of them in the same order if {@link Action#apply(Response)} is repeated.
+ * If all passed actions has been already applied it behaves like {@link #noop()} action.

This comment has been minimized.

@mkotsur

mkotsur May 9, 2016

Owner

Don't you think it should be configurable? I.e. Noop, or the first one, or a custom response like 404. It seems like noop can mask problems when all the responses are exhausted and the requests are still coming for some reason.

@mkotsur

mkotsur May 9, 2016

Owner

Don't you think it should be configurable? I.e. Noop, or the first one, or a custom response like 404. It seems like noop can mask problems when all the responses are exhausted and the requests are still coming for some reason.

This comment has been minimized.

@shamoh

shamoh May 9, 2016

Contributor

You are right it can be confusing. Do you have any preferred way how to configure it?

@shamoh

shamoh May 9, 2016

Contributor

You are right it can be confusing. Do you have any preferred way how to configure it?

This comment has been minimized.

@mkotsur

mkotsur May 10, 2016

Owner

I think, sequence can be complex enough to have its own fluent API. This will also keep the door open for other configs.

sequence(a1, a2).withDefault(defaultAction);

It shouldn't be necessary to define withDefault() though. then(sequence()) should still work with noop as default.

@mkotsur

mkotsur May 10, 2016

Owner

I think, sequence can be complex enough to have its own fluent API. This will also keep the door open for other configs.

sequence(a1, a2).withDefault(defaultAction);

It shouldn't be necessary to define withDefault() though. then(sequence()) should still work with noop as default.

This comment has been minimized.

@shamoh

shamoh May 13, 2016

Contributor

TBD

@shamoh

shamoh May 13, 2016

Contributor

TBD

@mkotsur

This comment has been minimized.

Show comment
Hide comment
@mkotsur

mkotsur May 9, 2016

Owner

Thanks a lot for the pull request. This is the biggest one so far!👋 I definitely think that thefeature is very useful, and restito needs it.

Please take a look at the comments and let me know what you think.

Btw, I am writing from the phone now, will get back to 💻 in a few days, please be patient :-)

Owner

mkotsur commented May 9, 2016

Thanks a lot for the pull request. This is the biggest one so far!👋 I definitely think that thefeature is very useful, and restito needs it.

Please take a look at the comments and let me know what you think.

Btw, I am writing from the phone now, will get back to 💻 in a few days, please be patient :-)

@mkotsur

This comment has been minimized.

Show comment
Hide comment
@mkotsur

mkotsur May 14, 2016

Owner

I've been thinking about this for couple of days, and then finally got back to the laptop, and could hack a little bit. The main concern now is that by keeping the queue inside the action object, we overload this (originally) very lightweight, well-composable and stateless concept.

This, for instance is perfectly fine now, but will become unsafe (because a may contain state, and requests to /bar will influence the action passed for /foo as well):

Action a = //something

whenHttp(server).match(get("/bar")).then(a);
whenHttp(server).match(get("/foo")).then(a);

How to avoid this? Keep configuration objects stateless, and push the state into higher-level concepts (like Stub, which already have appliedTimes, expectedTimes counters, thus can't be reused across requests, or to the StubServer, which holds the list of stubs).

I'm now doing some refactoring within Stubs-related code, based on your PR. It's still WIP, few tests fail, hope to finish it tomorrow to see how it looks in the end. Care to take a look?

https://github.com/mkotsur/restito/compare/shamoh-sequenced-stub-actions

Owner

mkotsur commented May 14, 2016

I've been thinking about this for couple of days, and then finally got back to the laptop, and could hack a little bit. The main concern now is that by keeping the queue inside the action object, we overload this (originally) very lightweight, well-composable and stateless concept.

This, for instance is perfectly fine now, but will become unsafe (because a may contain state, and requests to /bar will influence the action passed for /foo as well):

Action a = //something

whenHttp(server).match(get("/bar")).then(a);
whenHttp(server).match(get("/foo")).then(a);

How to avoid this? Keep configuration objects stateless, and push the state into higher-level concepts (like Stub, which already have appliedTimes, expectedTimes counters, thus can't be reused across requests, or to the StubServer, which holds the list of stubs).

I'm now doing some refactoring within Stubs-related code, based on your PR. It's still WIP, few tests fail, hope to finish it tomorrow to see how it looks in the end. Care to take a look?

https://github.com/mkotsur/restito/compare/shamoh-sequenced-stub-actions

@shamoh

This comment has been minimized.

Show comment
Hide comment
@shamoh

shamoh May 15, 2016

Contributor

@mkotsur Ok, let me know if I can help.

Contributor

shamoh commented May 15, 2016

@mkotsur Ok, let me know if I can help.

@mkotsur

This comment has been minimized.

Show comment
Hide comment
@mkotsur

mkotsur May 15, 2016

Owner

@shamoh @martinjmares are you guys using restito with Java8? I'm thinking about dropping 7 support. Is this fine for you?

Owner

mkotsur commented May 15, 2016

@shamoh @martinjmares are you guys using restito with Java8? I'm thinking about dropping 7 support. Is this fine for you?

@shamoh

This comment has been minimized.

Show comment
Hide comment
@shamoh

shamoh May 16, 2016

Contributor

Yes, we are on Java 8.

Contributor

shamoh commented May 16, 2016

Yes, we are on Java 8.

@mkotsur

This comment has been minimized.

Show comment
Hide comment
@mkotsur

mkotsur May 28, 2016

Owner

Created another pull request (#50) based on these changes. I guess, I don't have permissions to commit to this fork, that's why.

Owner

mkotsur commented May 28, 2016

Created another pull request (#50) based on these changes. I guess, I don't have permissions to commit to this fork, that's why.

@mkotsur

This comment has been minimized.

Show comment
Hide comment
@mkotsur

mkotsur Sep 28, 2016

Owner

The feature has been released and available in 0.9.0.

Owner

mkotsur commented Sep 28, 2016

The feature has been released and available in 0.9.0.

@mkotsur mkotsur closed this Sep 28, 2016

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