-
Notifications
You must be signed in to change notification settings - Fork 784
Add Location Strategy Keyword #365
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
Add Location Strategy Keyword #365
Conversation
|
@emanlove, could you take a look at this and give me your thoughts? Thanks. |
|
This is now feature complete and ready for review. I've added tests and documentation. It includes two new keywords:
The locator strategy itself is a user keyword as shown in the documentation. This allows easy integration with JavaScript (which was my original goal), but also covers any other applicable situation. An added locator strategy will stay present throughout the duration of testing unless removed by |
|
Global alterations of a testing environment by a test are generally unwise. @pekkaklarck, @emanlove what do you think of this behavior? I suppose it would be possible through library listener methods to automatically unregister the locator strategy after the scope it was registered in has subsided. Which would be better? The current behavior (after it's set anywhere it stays until it is removed) or the scoped behavior (where if it's set in a test it's removed after that test is complete). |
|
@zephraph, automatically removing locator strategies sounds like a good idea. Library listeners ought to make implementing it pretty simple. Should probably be configurable with an argument like |
|
I've added a specific test because I was in talks with @IlfirinPL on #376. This test case will likely be removed before merge along with the changes mentioned by @pekkaklarck above. Don't merge this as is. |
|
@pekkaklarck, when you get a chance could you review this? Adding the auto unregister made it a bit more complicated. |
|
@emanlove, could you please review this? Actually, look over everything in the 1.7 Milestone. I really want to go ahead and push a new release this weekend. |
|
A large part of the bulk of this is the scoping as suggested by @pekkaklarck. The original implementation made it persistent by default. This implementation makes it scoped by default (which required hooking into listeners). I try to make a little event system so if we needed to do stuff like this in the future we could just add new events. |
|
Sorry, I was on a business trip with limited time for other work. What commits/code should be reviewed? |
|
Just look here and see if anything jumps out at you. I really would like you to look over the event system I wrote for scoping the locator registration and tell me if it's too much. |
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.
Parens? Also in unregister below. #pep8
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.
Ah, good catch. That's my Javascript habits kicking in.
|
The event system looks pretty neat but I'm not entirely sure is it necessary for this usage. Are you planning to use it for something else too? If not, then there's a risk that YAGNI. I also added some nitpicking comments about style. I'm not sure is there any agreed styles for this project so feel free to ignore them. |
|
Well, it's quite possible I could use it again in the future, but I can't guarantee that. That's just how I thought to implement it. Also, I trimmed the scope events down a bit. It's more DRY now. |
|
Ok, looks good. |
|
So, are we good to merge? |
Added ```Add Location Strategy``` Keyword
|
I have an idea on similar enhancement: |
|
I'm not really following you. The custom locators written here should be specified in a resource file or library, not included in a test. I wrote this feature with javascript frameworks like angular and ExtJs in mind, though it's not limited to those sorts of things. I would say it's more up to the locator developer to document their locator usage. By and large this should be a rarely used feature though. |
This was discussed both in #333 and #364 and here's a preliminary thought that I had.
Instead of tying the
Add Location Strategyto javascript as I originally intended I've designed it so that a location strategy is actually implemented as a keyword. This gives a lot of flexibility in that location strategies can (if desired) be written directly in RF as a user keyword.Keep in mind that this is just a rough draft to get started with.
Idealistically I'd have this