Open laughingbiscuit opened 1 year ago
@laughingbiscuit thanks for proposal.
@ilslv would be nice to have this feature. Both .attach()
and .log()
(which finally mostly resolves our logging caveats).
It's also not obvious how images are rendered to console. Needs some further investigation.
Thanks @tyranron
In my experience, images do not need to be rendered to the console itself, but instead their base64 representation stored in the cucumber json and then they are subsequently displayed in the HTML report only. Here is an example.
@tyranron to be able to call .attach(...)
we need something to call in on. And this is a great opportunity to redo some stuff about how we extract arguments in Step
functions. I really like axum
's extractors approach:
// From this
#[given("step")]
fn step(w: &mut World, step: Step) {}
// Or this
#[given("step")]
fn step(w: &mut World, #[step] s: Step) {}
// To this
#[given("step")]
fn step(w: &mut World, Context(step): Context<Step>) {}
This is extendable, future-proof solution, that even will allow us to add user provided context, if we want to.
Also specifying only FromStr
arguments or &[String]
seems to me a bit too limiting, why not allow both:
#[given(regex = "from (\\S+) to (\\S+)(?:(?:,| and) (\\S+))?(?:(?:,| and) (\\S+))?")]
fn step(w: &mut World, from: String, to: Vec<String>) {}
// impl FromStr ^^^^^^ ^^^^^^^^^^^ impl FromIterator<Item = impl FromStr>
@ilslv
Also specifying only
FromStr
arguments or&[String]
seems to me a bit too limiting, why not allow both:#[given(regex = "from (\\S+) to (\\S+)(?:(?:,| and) (\\S+))?(?:(?:,| and) (\\S+))?")] fn step(w: &mut World, from: String, to: Vec<String>) {} // impl FromStr ^^^^^^ ^^^^^^^^^^^ impl FromIterator<Item = impl FromStr>
Yeah, this is definitely to have, but is a separate topic.
to be able to call
.attach(...)
we need something to call in on. And this is a great opportunity to redo some stuff about how we extract arguments inStep
functions. I really likeaxum
's extractors approach:// From this #[given("step")] fn step(w: &mut World, step: Step) {} // Or this #[given("step")] fn step(w: &mut World, #[step] s: Step) {} // To this #[given("step")] fn step(w: &mut World, Context(step): Context<Step>) {}
Could you sketch how it should look with .attach()
usage?
This is extendable, future-proof solution, that even will allow us to add user provided context, if we want to.
Actually, I don't understand, doesn't the World
represent a "user provided context" here?
Maybe we could wire .attach()
somehow to the World
?
@tyranron
Could you sketch how it should look with .attach() usage?
#[given("...")]
fn step(w: &mut World, cucumber::Extract(context): cucumber::Extract(cucumber::Context)) {
context.attach(...)
}
Actually, I don't understand here, doesn't the World represent a "user provided context" here?
I meant global user-provided context. For now docs are saying that it's not responsibility of this crate, but this may be changed in the future.
Maybe we could wire .attach() somehow to the World?
Basically, we need a way to get event::Cucumber
sender instance. This could be done with tracing
-like contexts via thread locals, but it introduces problems with spawn(async {})
, which I would like to avoid. So providing interface for extracting arbitrary types in steps is the only feasible thing to do in my opinion.
Also we should provide cucumber::Context
for before and after hooks
@ilslv I'm still quite confused with the notion of cucumber::Context
. We already have too many contexts here: World
, Scenario
, Step
. What is cucumber::Context
is about then?
If we look at JS implementation, it has .attach()
method on World
(usually called via this
). Java has it directly on Scenario
. Both feel rather straightforward and intuitive.
In our case, the Scenario
is actually a gherkin::Scenario
, so we cannot put such stuff here. Regarding this, I see the cucumber::Context
being defined like this:
#[derive(Deref)]
struct Context<T> {
#[deref]
inner: T,
// attachments and whatever
}
This way, we can add our cucumber
context to existing gherkin
elements:
#[given("step")]
fn step(w: &mut World, scenario: Context<Scenario>) {
scenario.attach();
scenario.steps; // accessible via `Deref`
}
Or maybe it's worth to attach the context to the World
?
#[given("step")]
fn step(w: &mut Context<World>, scenario: Context<Scenario>) {
w.attach();
}
Seem much more reasonable regarding &mut
signature.
What do you think on that?
@tyranron
What do you think on that?
I don't like the idea of making Context
generic over World
/Scenario
/Step
. I see it as a confusing way to hide complexity. cucumber::Context
is a separate entity and has nothing to do with World
or Scenario
.
We already have too many contexts here: World, Scenario, Step. What is cucumber::Context is about then?
But I understand and partially agree with point made here. The solution I see is to make our contexts hierarchical: user-defined World
and cucumber::Context
are equally important, but Feature
/Scenario
/Rule
/Step
should be retrievable only from cucumber::Context
as they are not really contexts in the same way.
We have 3 ways of implementing it:
World.context()
method, that will use ThreadLocal
variable to share Context
. It's a non-breaking change and I like simplicity of it, but problem with loosing ThreadLocal
context because of thread::spawn
/tokio::spawn
is unsolvable without special wrapper, which I would like to avoid. #[step]
/step: Step
arguments, use _: Context
(we can retrieve it using auto-deref specialization).All in all my proposal moving forward is following: replace #[step]
/step: gherkin::Step
with more general-purpose any_ident: cucumber::Context
. This is a breaking change, but we can detect places where replacement is needed and emit understandable compile-time error.
@ilslv if we make a cucumber::Context
as a struct, consisting of gherkin::Step
, gherkin::Scenario
and some extra, seems quite reasonable. So, in places where we pass Scenario
, Step
and all the shitstuff, we can pass only the cucumber::Context
, simplifying the signatures in many places. At the same time, if the user needs only Step
we still can provide it to him directly, in a price of some simple under-the-hood extraction machinery on top of the cucumber::Context
type.
Sounds reasonable. The only thing that feels strange here is the mutability question. As for now, Step
/Scenario
are read-only fields, and the only mutable context passed to the user is World
which has the explicit &mut
signature. If we put the mutable cucumber::Context
into play, we should make either both via &mut
, or both interior mutable. 🤔
@tyranron
If we put the mutable cucumber::Context into play, we should make either both via &mut, or both interior mutable.
I don't quite understand this point. The way I see it, World
should be mutable as we have exclusive ownership over it and cucumber::Context
should be immutable, as multiple Scenario
s can be executed at the same time.
Looking through the docs, I cant see an attach dunction like in cucumber jvm/js/rb. The purpose would be to attach images or assets to the cucumber json result for display in a report.
Any thoughts?
https://github.com/cucumber/cucumber-js/blob/main/docs/support_files/attachments.md
https://javadoc.io/static/io.cucumber/cucumber-java8/6.9.1/io/cucumber/java8/Scenario.html#attach(byte%5B%5D,java.lang.String,java.lang.String)