-
Notifications
You must be signed in to change notification settings - Fork 69
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
[WIP] Add async tests #32
Conversation
728d500
to
63ef56f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
I like this because IMHO the main advantage of async/await as a lang feature is that "normalizes" the usage of async code. Ideally, a user of cucumber can expect async functions to just work. The amount of code to maintain will only increase with merging this, sure, but implementation still seems pretty straightforward.
I've noted a few minor things that could be refactored, but I've not read every single line. Maybe some reorganization can make parts of this more readable, too, like representing the test types like {sync,async}x{static name,regex}
(I think you already mentioned that when talking about proc macro stuff) and introducing traits to "overload" the builder API and reduce the API surface.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
My current work on this, inspired by @humb1t, is on the |
Updated the The old branch is now |
Would love to see async support. Without it I cannot use the crate for a specific project :( |
I would too, but time is a dictator! 😄 What's your specific use case? I might be able to help with a workaround or other advice. |
I understand the time constraint, no worry! I would like to automate some end-to-end testing of a complex system which includes interacting with a webapp, Azure cloud and a Windows machine (🤯). I can manually do this as I have all the required building blocks, but I would have preferred having a kind of framework to dictate some kind of structure to this testing, including nice output and stats about failures and success. I think it fits nicely in BDD and The building blocks I mentioned are async (tokio) so I absolutely need a runtime to drive them. |
@nbigaouette I'll be working on this again on the weekend, so I'd appreciate it if you have another try next week. If I get it done, there'll be documentation, and I'll try to ensure that your use case actually works. 😄 |
@nbigaouette I added an |
Thanks for this! I unfortunately did not have time to put on this last week. I've opened a PR (#51) with a simple modification to the method. You can see the compilation failure in CI: https://travis-ci.org/github/bbqsrc/cucumber-rust/jobs/690704653 Before trying out cucumber, I expected having a single I could try to work on a immutable self to bypass this, but I know I did stored some kind of states between the steps (f.e. number of items found in the system before an action is taken). |
I'll have a look, thanks! |
@nbigaouette I have pushed to develop, does the change I made to cucumber_builder help? |
@nbigaouette also really really want to change CI from Travis to Github Actions, if you have any interest in doing that, I'd appreciate it (I do enough CI for my day job that any moment not thinking about this is a moment of bliss hahaha) - made an issue for that #52 |
Ok, so wrapping the async block in a Will that be required for or is there something more ergonomic in the pipeline? I'll next try the develop branch in my project... |
@nbigaouette this is, shall we say, the "primitive" implementation. Once we've got this down, I will make a procedural macro that hides the dirtiness so we can write tests more naturally. Something like As they say, premature optimisation is the root of all evil. 😄 |
After playing with it I have few notes about the events regarding Scenario Events API. Namely consider: #[derive(Debug, Clone, PartialEq, Eq)]
pub enum ScenarioEvent {
Starting,
Background(Rc<gherkin::Step>, StepEvent),
Step(Rc<gherkin::Step>, StepEvent),
Skipped,
Passed,
Failed,
} Every step and background ends in one way or another, there is no limbo for them. And yet if you want to handle either of the three possible handles you must tackle it as separate event, one which doesn't get the details about what has failed, as this is only provided in Background and Step. This means that if you want either of the finishing Events to know what is it working with, you must manage the context yourself. That has a long term issue if we ever want to create parallel runners as managing this context will become problematic. Although more importantly I think the API right now could be much better if we simply reduced it to structure like this: pub enum ResultType {
Success,
Failure,
Skipped
}
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ScenarioEvent {
PreScenario,
Background(Rc<gherkin::Step>, StepEvent, Result: ResultType),
Step(Rc<gherkin::Step>, StepEvent, Result: ResultType),
PostScenario
} And similar implementation for the steps. This way you are only processing the actual thing that happened (scenario executed, step ran, etc) and have all the context needed available right away, without the separate actions multiple events. I think that would be a much clearer result and save us a lot of annoyance down the line. |
Mm, I think in either case managing context is going to be necessary, due to the interplay between rules and scenarios. I can, however, have the failed step propagate its failure to the Failed arms of all the Scenario and Rule events. I have also considered parallelization while designing this. My decision was that the synchronisation point would be up to the stream sending the output events, so in a parallel mode, you would get a serial batch of step events for each scenario that runs. So in that case, it would also still make sense for a ScenarioEvent to have some kind of StepsSummary argument for the conclusion events. I'm also wary of separating out the completion result into its own type, we had done something similar before and it became rather complicated as distinguishing between a step result from a scenario result from a rule result was a bit of a hassle. That said, I'm glad you brought this up. I'll mull over it and consider some changes on the weekend. |
Playing again with this (04c3475 on My world now looks like this: pub struct MyWorld {
member: Arc<RwLock<MyOtherStruct>>,
// ...
} It's a bit unfortunate and ugly, but I'm ok with it. I tested the given/when/then with I saw something weird in the feature file; if I "comment" some lines (with
I guess the file parser does not support those types of comments. Another observation is that all output to stdout/stderr is hidden; I thus cannot see any logs. I'll switch from env_logger to one that saves to a file. It would be interesting if cucumber could redirect itself outputs to a file. I'll port the remaining of my tests to see how it goes. But I like what I'm seeing! Good job! |
So one of my idiosyncrasies is that when I write a grammar, I tend to forget to handle comments properly at all (and I've written about 3 grammars this month, all displaying the same bug... haha). I'll look into that one the weekend. Redirecting the stdout and stderr to a file is actually possible, I'll put it on the backlog! I'm glad you're enjoying it. Thanks for the feedback! |
Forget to say... Constructing
|
Haha, yes, I took a shortcut there, knowingly! Not surprised you came across that, given you've found every other edge case and lazy option I took. I'll look at that on the weekend too. xD |
Haha :D If we weren't lazy, we wouldn't automate our testing, wouldn't we? ;) |
look |
eurg.... I'm still hit by the borrowing issue:
Here's the code: .given_regex(
r#"Name name="(.*)" is present"#,
Rc::new(|world, matches, _step| {
std::panic::AssertUnwindSafe(async move {
let name = &matches[0];
let mut tmp = world.member.write().unwrap();
let roles = tmp.async_method_taking_mut_self().await;
world
})
.catch_unwind()
.boxed_local()
}),
) I need a temporary variable or else I get a |
Something like https://docs.rs/tokio/0.2.21/tokio/sync/struct.RwLock.html may help here. Holding a lock across an await boundary is often not permissible, and my guess is that might be the issue here. It might also be that you need to do: let tmp = Arc::clone(&world.member);
let mut tmp = tmp.write().unwrap(); |
The |
@nbigaouette there is a difference between a tokio RwLock and a std RwLock though, as the tokio one is asynchronous. It has some cases where using a std one will not work, though I can't remember the precise case I needed it, but it was within the last week heh |
Haaa good to know, I'll keep that in mind if I have another issue. I'm using a std lock for now. |
@nbigaouette World is now failable (it will crash right now, just made it unwrap, but will make it handle panics and errors properly), and gherkin has been fixed to handle spaces in front of comments. :) |
Thanks! I want to try it but can't for now. the error type of World trait is #[async_trait(?Send)]
impl cucumber::World for AccessGovernorWorld {
type Error = anyhow::Error;
... I get the following:
See for example/reference:
Not sure what the best approach would be here. Should I handle this myself (creating a new error type that implements Error) or can cucumber be more flexible for this? It's kind of weird that |
Workaround here: dtolnay/anyhow#63 (comment) |
I've begun adding async support to Cucumber.
Async test running functionality
Async ergonomics
async
testsasync regex
tests