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

add specs for TestRunStarted message #613

Open
wants to merge 7 commits into
base: master
from

Conversation

@david1995
Copy link
Contributor

commented May 9, 2019

Summary

This is the start of writing specs for Cucumber messages as discussed on the Community Day on the CukenFest.

Details

First scenarios for TestRunStarted message.

Motivation and Context

All implementations of Cucumber should behave the same way.

@SabotageAndi

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

This is a first draft for the specs. Feedback is very welcomed, as we are not 100% sure on the wording.

@luke-hill
Copy link
Contributor

left a comment

Seems solid. I've not commented on grammar / tenses as I don't know the conventions you have in .NET

Scenario: Handling a message queue with no TestRunStarted message
Given there is no TestRunStarted message in the message queue
When the messsage queue is processed
Then there is an error

This comment has been minimized.

Copy link
@luke-hill

luke-hill May 9, 2019

Contributor

Here should we elude to the fact that it's an error complaining about TestRunStarted - Not sure how you could word it....

Then an error is shown that TestRunStarted is not present in the queue
Then a TestRunStarted error is thrown
Then a TestRunStarted error is generated
Then there is an error suggesting TestRunStarted is not present

This comment has been minimized.

Copy link
@david1995

david1995 May 10, 2019

Author Contributor

Your first suggestion sounds best to me. What do you think about Then an error indicating that no TestRunStarted message could be found in the queue should be shown?

Show resolved Hide resolved cucumber-messages/specs/receiving/TestRunStarted.feature Outdated
Scenario: Handling a message queue with two TestRunStarted messages
Given there are two TestRunStarted messages in the message queue
When the message queue is processed
Then the second TestRunStarted message has been ignored

This comment has been minimized.

Copy link
@luke-hill

luke-hill May 9, 2019

Contributor

Should this check that the start of the run was recognized?

This comment has been minimized.

Copy link
@david1995

david1995 May 10, 2019

Author Contributor

Yes, this check should not be omitted. @SabotageAndi what do you think?

This comment has been minimized.

Copy link
@david1995

david1995 May 10, 2019

Author Contributor

I've added the same Then statement as in the scenario above, which should clarify the behavior.

@luke-hill
Copy link
Contributor

left a comment

Sounds solid. Again, some perhaps grammatical stuff I'd tweak but this is definitely a good start. 🎉

@gasparnagy gasparnagy marked this pull request as ready for review May 16, 2019

@gasparnagy
Copy link
Member

left a comment

I think it is very hard to see how we want to progress with this because the TestRunStarted event is too simple. I suggest adding at least one other (preferably one with a payload) so that we can better judge.

What I see already:

  • I would consider wether we really need to specify the receiver side. I think we primary focus on the events that the cucumber implementations should raise. What external tools will do with the result is rather the scope of the external tool itself (e.g. whether they report errors or just silently ignore).
  • I see that you defined a term test suite. I am fine with this, but we should check if there was a term in use already by cucumber for the same.
  • the Given there is a feature file actually means that there is a feature file in the test suite. Maybe it would be better to say Given there is a test suite with a feature file. (If we want to keep the test suite concept.)
@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

In Cucumber JVM the test run started event has a payload, a timestamp to indicate when the run started. Everty other event also has a timestamp.

So we could add a ISO 8601 time stamp. The time should always be in UTC. 2019-05-18T07:18:58Z

This also raises a few questions:

  • Do we want to include that now or start with a truely minimal spec.
  • Do we allow extensions to the spec?

I also think we are missing some principles about the channel. How can it be used? Currently only the channel can only be used by a single cucumber executor at a time. Is this intentional?

If we are to allow concurrent execution of pickles, why also not concurrent execution of executors?

Either way, adding an executor UUID to each event would allow us todistinguish the events from different executors on the same channel.

@SabotageAndi

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@mpkorstanje We have another branch where we add a timestamp to the TestRunStarted message. We didn't send it yet as PR, because we wanted first to get feedback on the formulation of the specs, so we don't have to redo the specs for it multiple times.
We will send a PR for it in the next hours.

About the executor UUID: would be fine for me. Reason is, that because of how the SpecFlow+Runner is implemented, we will send multiple TestRunStarted messages (one for each Thread). With that, we could differentiate them.

@gasparnagy
Yes, these scenarios are a little bit too abstract. The next PRs will be more concrete.

After discussion here, we removed the receiving side. We wrote it because of the behaviour of the SpecFlow+Runner (multiple messages). We solve this now with a different scenario.

About the term testsuite: that's one of the reason for this small PR. To get the first feedback to the problem domain language.

@SabotageAndi

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

PR for adding timestamp is here: #615

@mattwynne

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I'm really glad to see us using feature files to specify these messages!

I'd like to see some concrete details about what is in the message. I would prefer to see something like #615 as the first iteration, rather than having this very SpecFlow-specific stuff in the second scenario about running in parallel.

@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

About the executor UUID: would be fine for me. Reason is, that because of how the SpecFlow+Runner is implemented, we will send multiple TestRunStarted messages (one for each Thread). With that, we could differentiate them.

@SabotageAndi now I'm a bit confused by your explanation. Say I have 12 pickles and I execute them in parallel on 3 threads. Will this mean that these are separate 3 test runs of 4 pickles.How would a listener to the event stream know that the original 12 pickles are all done?

This sounds like a rather significant difference. In Cucumber JVM one executor sends a single started/finished event and spins up several threads to execute the pickles in between these events. It is possible to have multiple executors running in parallel. They'd be executing different runs.

In a diagram:

System --- 1...n --> Cucumber Executors -- 1...n --> Threads
System
 |- Executor 1 executing pickles 1...12 tagged with @foo
 | |- Thread 1.1 Executing pickles 1...4
 | |- Thread 1.2 Executing pickles 5...8
 | |- Thread 1.3 Executing pickles 9...12 
 |- Executor 2 executing pickles 1...12 tagged with @bar
 | |- Thread 1.1 Executing pickles 1...4
 | |- Thread 1.2 Executing pickles 5...8
 | |- Thread 1.3 Executing pickles 9...12 

@mattwynne I realize this is rather abstract but I've had a hell of time retrofitting concurrency into Cucumber JVM. If we don't get this right from the ground up it will be very painful in the future. And I guess I'm less interested in the other parts of the protocol, the idea of using events has already been proven.

@aslakhellesoy

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

I think it's great to see progress towards a shared test suite for all Cucumber implementations.

Cucumber messages are fairly low level implementation details, and I am not convinced Gherkin is the best way to express how the messages should be constructed and processed. It feels to me like some sort of approval test suite is better suited. This is the approach taken for Gherkin implementations.

We also need to distinguish between the messages themselves and the Cucumber implementations producing/consuming these messages. This .feature file seems to be about validating the latter (a Cucumber implementation). The monorepo doesn't host any implementations (yet), so there is no way to actually run this feature file in the monorepo. Developers could easily introduce inconsistencies between the .proto files and the .feature files without noticing they have broken something. This violates an important principle in my book: tests should be executable and fail when something is wrong.

I think we need to take a step back and agree on what we want to achieve with a shared test suite before we start implementing it.

  • What kind of confidence are we looking for?
  • What parts of a Cucumber implementation are we looking to test?
  • What's out of scope of the test suite?
  • etc...

aslakhellesoy added a commit that referenced this pull request Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.