Make integration tests for content entity rest endpoints multithreaded again #2124
Conversation
| AfterCreateEvent afterCreate = new AfterCreateEvent(savedEntity); | ||
| publisher.publishEvent(afterCreate); | ||
|
|
There was a problem hiding this comment.
This is not only changing the integration tests if you're moving the place where this event is emitted.
There was a problem hiding this comment.
This is true. I am not sure I have a strong opinion in this case. I cant thing of a reason why the publish cant be at the end of the call, rather than just after the event. I dont think it materially effects the behavior. WDYT?
There was a problem hiding this comment.
The difference is in the ordering of events.
Previously, the AfterCreateEvent is emitted before the set content events (which I believe are emitted internally inside the ContentStoreContentService methods).
Now, the AfterCreateEvent is emitted after the set content events.
It is a behavior that I think should explicitly be called out, not just sneaked in together with some changes in integration tests.
There was a problem hiding this comment.
Right. I would agree. But I doubt many folks are using this controller yet besides you folks. But that is valid. The only reason for this change is because of my general desire to support multi-threaded tests but we lapse in a bunch of places so if you are not comfortable with the event ordering changing then we probably should force it through in a test change.
| @@ -40,7 +42,7 @@ | |||
| import com.github.paulcwarren.ginkgo4j.Ginkgo4jSpringRunner; | |||
|
|
|||
| @RunWith(Ginkgo4jSpringRunner.class) | |||
| @Ginkgo4jConfiguration(threads=1) | |||
| //@Ginkgo4jConfiguration(threads=1) | |||
There was a problem hiding this comment.
Remove commented code
I added some tests that rely on threads=1, so I figured I'd fix that.