-
-
Notifications
You must be signed in to change notification settings - Fork 144
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
Adding ordering in Before hook #1128
Conversation
lib/registry.ts
Outdated
(a: ICaseHook, b: ICaseHook) => { | ||
// If order is not specified, the hook will be executed at the end | ||
if (a.order === undefined) { | ||
a.order = Number.MAX_SAFE_INTEGER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default order should be made to 10000 in order to mimic behavior seen in other implementations, ref. cucumber/cucumber-jvm@1a5e98d.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/registry.ts
Outdated
@@ -65,6 +65,7 @@ export interface ICaseHook { | |||
position?: Position; | |||
tags?: string; | |||
name?: string; | |||
order?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this required and give it a default value during assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, addressed this
Order is an option in all hooks in the jvm implementations and should such be added here given the goal is conformity. |
Each hook deserve more comphrehensive testing than this and requires separate tests (ie. don't mix it with "the big one"). I imagine essentially three scenarios for each hook:
.. with similar tests for each other type of hook. This can be accomplished using X scenario outlines. |
Thanks for your feedback, @badeball! So ordering has to be implemented in all of these - Also, I understand we reverse the order of After hooks |
Yes, all hooks and yes, reverse sorted in all after-type hooks! |
Is it a good idea to put all of these tests in the same feature file (hooks_ordering.feature)? Or should I create a separate feature file for each hook? |
Yeah, one feature file is fine for this purpose. |
Addressed your comments @badeball. Please let me know your thoughts on it! |
Primarily introducing scenario outlines as a shorter alternative.
Excellent, thank you. I've merged and released this with v19.2.0. For reference, you can check out 1e3643a to see how I used scenario outlines to shorten everything. |
No description provided.