reqnroll / Reqnroll

Open-source Cucumber-style BDD test automation framework for .NET.
https://reqnroll.net
BSD 3-Clause "New" or "Revised" License
357 stars 37 forks source link

Added order parameter to stepArgumentTransformation to prioritize execution #185

Closed stbychkov closed 2 months ago

stbychkov commented 3 months ago

🤔 What's changed?

⚡️ What's your motivation?

In .NET 6 and earlier versions, the GetMethods method does not return methods in a particular order, such as alphabetical or declaration order. Your code must not depend on the order in which methods are returned, because that order varies. However, starting with .NET 7, the ordering is deterministic based upon the metadata ordering in the assembly.

(source)

Type.GetMethods is used to find all StepArgumentTransformation functions, but the order is not deterministic before .NET 7. This inconsistency made it tricky to handle type transformations, especially when setting up global handlers for optional parameters.

🏷️ What kind of change is this?

📋 Checklist:

ajeckmans commented 3 months ago

I think the example that is added to the docs highlights another issue. I think it makes sense to match in more specific transforms first and then more generic later makes sense.

So adding an implicit ordering/priority as follows would make sense to me:

I can see a need for attaching a priority to a given transform within each of those categories, but I think adding the above would make sense regardless.

Also ordering as a term is a bit too technical and tied to the implementation for my liking so I suggest renaming that to priority. Where a higher priority means it is chosen first.

stbychkov commented 3 months ago

Thanks for the feedback, @ajeckmans! I'll make that change when I get a chance. Feel free to tweak it to fit your vision.

Where a higher priority means it is chosen first.

About the priority order: I actually prefer keeping the lower values with higher priority to stay consistent across the solution. You can see the same pattern in HookAttribute. I believe that introducing a new strategy to set the priority will lead to confusion and inconsistency, what do you think?

ajeckmans commented 3 months ago

Makes sense. Let's hold back from making changes until @gasparnagy also looked at the PR. I think he will have something to say about this as well :)

gasparnagy commented 3 months ago

@stbychkov @ajeckmans Just a quick note: the Order property of the hook attributes was chosen because of cucumber compatibility. With hooks "priority" does not make sense, because it is not defined whether the hooks that run first have more priority or the once that run later (as they can override the effect of the others). So Order made sense there.

In case of the StepArgumentTransformation, the term priority is more appropriate, but it will still be unclear if the lower number is more prior or the higher number. So the order is probably more clear (and also consistent with the hooks). Basically it means: this is the order how we evaluate the transformations.

I am generally fine with the idea. I will do a proper review (latest on Monday).

I need to think more on the comment of @ajeckmans https://github.com/reqnroll/Reqnroll/pull/185#issuecomment-2169157281

gasparnagy commented 3 months ago

@stbychkov pls try to fix the build in the meantime

gasparnagy commented 2 months ago

@stbychkov Thank you for the contribution! According to our guidelines I have invited you to the Reqnroll contributors team. Congrats! 🎉 If you accept it, you will be able to make pull requests easier in the future.

You are also welcome on our discord server: https://go.reqnroll.net/discord-invite