Skip to content
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

Exploration | Macro for validating xpaths/css selectors | ORM/builder for composing xpaths #82

Open
bcpeinhardt opened this issue Jan 3, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@bcpeinhardt
Copy link
Contributor

I wanted to follow up on the macro discussion. I was thinking a procedural macro that doesn't actually change any code (it wouldn't even use parse_macro_input!() because TokenStream implements ToString). Basically we'd build a String from the TokenStream, take the chance to panic if we don't like what we see, or just pass the TokenStream right back if everythings alright. I have a basic setup at https://github.com/bcpeinhardt/valid_selectors although I'm still working on nice error handling. It's hard to find 15 hours to get through @jonhoo s procedural macro videos haha.

As far as actually validating xpaths maybe something like https://crates.io/crates/libxml. It seems to have an XPath validator separate from the context, and if it needed features in the future I honestly think it would be easier to contribute to bindings than maintain FFI to anywhere. Haven't looked into the CSS selectors yet.

@stevepryde
Copy link
Owner

Yeah the behaviour I was imagining was that it would simply underline an invalid xpath in red at compile time, hopefully with a nice error message (returned from libxml?). At runtime it should be a no-op or as close to that as possible.

I don't have time to look at it right now but will get to it soon.

@stevepryde
Copy link
Owner

Also, previously a colleague and I built separate ORM-like builders for XPath, one using a builder pattern and the other using operator overloads. What we found was that while it made writing XPaths easier, the code was a lot more verbose and sometimes more difficult to parse than the equivalent xpath string.

Writing xpath was easier because you could use intellisense to guide you rather than having to remember all of the specific xpath syntax. So there's still some positive benefit here.

But I guess my point is to mention the potential readability cost of an ORM as well. The macro is entirely separate from an ORM though so I think if we do decide to go down the ORM path that will be a separate issue.

@bcpeinhardt
Copy link
Contributor Author

Yeah to be honest for me the query builder is pretty great, and I don't feel an overwhelming need for anything more powerful. The reasoning behind using something like that in my mind is to reuse a smaller more manageable set of locators per component/page object, and perform the sort of predicate checks (like querying with and_clickable() for a button or something) in the relevant methods without requiring the use of different paths for the same element, not to try to intellisense my way out of remembering how xpaths work, especially when just the basics of xpath pretty much serve most automation needs.

Assuming other people disagree and would like a type checked way to build up xpaths, the way I see the macro relating to an xpath builder or set of operator overloaded types would be as a safeguard against misuse, so if a nonsensical xpath is constructed the macro catches it. Ideally the api wouldn't be capable of constructing a "bad" path, either through lots and lots of types and type constrained generic methods or a general logical structure to how the path is "enriched" during its construction, but I suspect that will take trial and error to find.

@bcpeinhardt
Copy link
Contributor Author

bcpeinhardt commented Jan 6, 2022

@stevepryde so the basic idea behind https://github.com/bcpeinhardt/valid_selectors is up and running (for xpath). I have a pull request to get the binding we needed into the libxml crate but right now its on my fork. The actual c function we call (http://www.xmlsoft.org/html/libxml-xpath.html#xmlXPathCompile) prints an error message which I'm not sure there's anything we can do about, but it's not bad looking.
It'll need some testing to figure out exactly what checks it's doing, but hope this checks an item off your wish list.

@stevepryde
Copy link
Owner

This is awesome.

I'm a little concerned about the dependency requirements of libxml though. Proc macros need to go in their own crate anyway so there's no need to put it behind a feature flag. Maybe that is ok, any users who want to use it will need to decide whether to incur the dependency cost. It only needs to be a dev dependency at least.

What are your thoughts on that? Do you happen to know if there are any pure-rust alternatives you could use?

@bcpeinhardt
Copy link
Contributor Author

bcpeinhardt commented Jan 6, 2022

Completely agree, no good reason to have this as a non dev dependency, and certainly no need to build it into thirtyfour. No, on the crates page for the library the author actually says they wrote the bindings to hold developers over until there is a mature rust crate that can serve as a drop in replacement. I think (once I've managed to actually outline the spec the macro tests in my own test cases) I'll publish this as a separate crate and call it like
By::XPath(xpath!("//a"))... for the peace of mind.

Once there is a nice open source pure rust version of this and we could just strip the xpath validation, or I have the time and inclination to write such a behemoth myself, then I could see trying to make this somehow integrated with thirtyfour.

@bcpeinhardt
Copy link
Contributor Author

bcpeinhardt commented Jan 6, 2022

This is the most serious attempt at a Rust equivalent to libxml that I found on crates.io
https://github.com/shepmaster/sxd-xpath

@bcpeinhardt
Copy link
Contributor Author

Well hold on now I dismissed it when I saw build failing and the older commits but I'm going to give this a once over. I spy an xpath parser and a text file of xpath grammar.

@stevepryde
Copy link
Owner

I'd be happy to re-export your xpath macro behind a feature flag if it requires libxml, but if you can get a pure rust solution working that's definitely preferred (and then it should be fine without a feature flag).

@bcpeinhardt
Copy link
Contributor Author

That sounds good! I'll follow up here when I figure out everything that's actually being checked at compile time and when the binding gets merged. It really bugs me that I can't go read the internals of the library, but maybe there's some way I can reach out to at least get a description that I can verify with some tests.

@stevepryde stevepryde added the enhancement New feature or request label Jul 31, 2022
@bcpeinhardt
Copy link
Contributor Author

I really dropped the ball on this one, sorry about that. They did end up merging that additional binding. I still think a sort of validate_xpath! macro is a decent idea, especially now the Component structs with a bunch of locator fields is a built in concept of the crate.

@stevepryde
Copy link
Owner

Oh don't stress. It's all fine. I appreciate the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants