Skip to content

Add execution order test#63

Closed
kelunik wants to merge 3 commits intoreactphp:4.xfrom
kelunik:execution-order
Closed

Add execution order test#63
kelunik wants to merge 3 commits intoreactphp:4.xfrom
kelunik:execution-order

Conversation

@kelunik
Copy link

@kelunik kelunik commented Aug 27, 2022

This test currently has a different execution order in my adapter, so let's document the expected execution order.

This test currently has a different execution order in my adapter, so let's document the expected execution order.
Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kelunik, thanks for your work on this 👍

I suggested a few changes below. Can you also place this test a bit lower in this test class. We try to have the easiest/simplest tests at the top and it progresses in complexity as you go down.

Co-authored-by: Simon Frings <simon.frings1@web.de>
Co-authored-by: Simon Frings <simon.frings1@web.de>
@kelunik
Copy link
Author

kelunik commented Sep 7, 2022

@SimonFrings Done. 👍

@SimonFrings
Copy link
Member

@kelunik thanks for the update 👍

The test itself in its functionality and the overall design looks great. I talked with @clue about this and it looks like your testcase is already covered by other tests and didn't raise the overall code coverage.

I would argue that it wouldn't make sense to add a test just to show a specific example of what is possible.

@clue
Copy link
Member

clue commented Oct 24, 2023

I'm closing this for now as it hasn't received any input in a while and I believe this has been answered. If you feel this is still relevant, please come back with more details and we can always reopen this 👍

Thank you for your effort nonetheless, keep it up! 👍

@clue clue closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants