-
-
Notifications
You must be signed in to change notification settings - Fork 694
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] event-protocol: Add events & schemas for test runs #172
Conversation
091bffa
to
be6683b
Compare
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.
This is great @matt - love that they are consumer-driven.
I only have one piece of feedback, and that's about line
, column
and uri
, and how we group those and what we call them.
I think it's important to develop a ubiquitous language here.
Gherkin uses uri
as a single field to point to a Gherkin document. It's typically a relative file system path, but it could also be an absolute path, or even a URL.
It also uses location
to refer to a line
,column
pair within a document identified by a uri
.
In other words, we use the term uri
as the address space for documents, and the term location
as the address space of lines and columns within a document.
The introduction of sourceLocation
mixes those concepts. Now a uri
is "inside" a location, which conflicts with the existing usage described above.
So instead of this:
"sourceLocation": {
"uri": "features/passing.feature",
"line": 2
}
I suggest this:
"source": {
"uri": "features/passing.feature",
"location": {
"line": 2
}
}
We could make the column
property of the location
type optional.
Thanks for the feedback @aslakhellesoy. You raise an interesting and valid point about ubiquitous language. In cucumber-ruby, we've been using the term "location" to refer to this combination of file-URI-and-line-number (the thing we used to call I'd be happy to use a new name for it, but I do still think this concept - the combination of the coordinate position of the element within a file, and the location of the file itself - deserves a name of its own. It's very useful within a full set of test cases / features as the unique locator key for a given element or thing you want to be able to find. That's why we've always used The other puzzle I have around this is that we need to not only be able to use this to name the I guess we could just call those two attributes My instinct would to use the name |
Test Steps can be created from:
In case of Test Steps from Scenario Outlines, it would make sense to only represent the In Ruby and other interpreted languages |
@brasmusson I love the clarity you bring to these things! You're right that a pickle from a scenario outline examples table row needs two source location pointers for us to be able to correctly identify the source. As a unique ID, however, we only need the Gherkin document's URI and the line of the row in the examples table, right? So perhaps we need to stop conflating the "location" concept with the "identifier" concept - it's the latter I'm looking for here. The idea is that these test result messages just contain identifiers/pointers into the pickle / ast data that's already been sent. Once I have the identifier, I can use it to look up the pickle, which will tell me all the details of the source locations. I hear you about |
In case of a Test Case, only the line of the Example Table row would be sufficient, but it inconsistent with the Pickle which have both the line of the Example Table row and the line of the Scenario Outline row:
In case of a Test Step both the line of the Example Table row and the line of the Outline Step are needed, the latter is essential to determine the Keyword of the step (which is not represented in the Pickle, and therefore cannot be known to the Test Step either).
This is a copy of the pretty formatter printout for a scenario of the java-calculator example in Cucumber-JVM:
|
Yeah, I wonder if we should just rename that event to be a bit more broadly applicable than just a
Essentially, this stream of events has been caused by us kicking off some kind of task / command / operation. What would be a better name for that? |
This prepares the way for keeping protocol formatters / emitters together in the event-protocol folder too.
This illustrates the problem Bjorn is talking about in #172 (comment) where we need to use two lines to represent the location of a scenario outline step but we're currently only using one.
This illustrates the problem Bjorn is talking about in #172 (comment) where we need to use two lines to represent the location of a scenario outline step but we're currently only using one.
6ccb76e
to
15f9e57
Compare
I've now merged in the "exemplar" Ruby formatter into this branch from #173. It's helpful as I can use it to generate more examples. I've updated the checklist with what I've identified as the remaining puzzles in this PR. I'm currently working on having better output around the location of scenario outline steps, as suggested by @brasmusson. Work in progress. |
Concerning location and uri, is there any reason why we are not following the RFC for URI scheme like
In order to produce events. |
@l3pp4rd what you suggest makes sense. I feel as if relative paths would be better than absolute ones, since we really don't care about (and it almost feels a bit intrusive to broadcast) the users's containing folder structure. Are you allowed to use relative paths in formal RFC-compliant URLs? |
@mattwynne with relative paths I only meant that gherkin CLI tools can treat it as usual files as before, but |
4daf199
to
24f7335
Compare
I'm going to propose that we merge this as-is, and deal with any subsequent changes as separate PRs. It seems we probably need to start using it in earnest before we can iterate on it much more. This will mean @charlierudolph build his JS implementation off of master, rather than off of this PR. We can still treat the overall protocol as draft even if we've merged this PR. Any objections? Any last concrete suggestions for changes before this gets merged? |
I think that the In general I expect test-case event and test-steps event to be like the pickles and pickle steps - plus some additional information. |
Ok @brasmusson so we'd repeat the I'm fine with doing that 👍 as long as we also keep the pointer to it's position within the test case overall. |
@aslakhellesoy @mattwynne @brasmusson On cucumber-js, it was easy enough to get by without having sourceLocation, actionRef in the test-step events. I like the minimalism in the proposed events of this PR. The built in formatters use an "event data collector" which listens for stores almost all the event data in a way it can be queried easily enough. Couple events I added so far as I am implementing the event protocol
|
When defining events there is no doubt a lot of both philosophy and assumptions embedded in those definitions. On the philosophy side there is the decision of to what extent, or which, events should be self contained. Some event should obviously (?) be self contained, like the Pickle event (which contains everything from the feature file needed to execute the Pickle), for many others its a matter of philosophy. One extreme is to avoid as much duplication between different events - which means that listeners need to collect basically all events of every event type to do something at all. The other extreme would be to make the events more or less self contained, so listeners need to listen to only a few of the event types to get the information they need. In Cucumber-Ruby 2.0 the new formatter API (API calls later turned into event receptions) of An example of a design assumption going into the definition of an event, is the inclusion of the number of test cases to be executed in |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. |
@mattwynne what needs to be done to get this merged? I think is a great start and we can make improvements on this as we use it and build on top of it. |
@charlierudolph I'd say we could merge this as-is and then iterate from there. Next steps I think would be:
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. |
This has gone stale since we decided to go for protocol buffers. Protocol buffers have a built-in schema, so this is kind of solved anyway. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Add events to describe and monitor test runs.
Details
This PR adds a set of new events that describe the execution of a test run:
test-run-started
test-case-prepared
test-case-started
test-step-started
test-step-finished
test-case-finished
These were consumer-driven from the prototype GUI in #171.
Motivation and Context
These events allow us to build cross-platform formatters and other tools that process test results. The existing JSON format is messy, hard to write and hard to read. These events will replace them.
How Has This Been Tested?
The schemas are tested using
make
which runs each of the example events through the validator.Types of changes
Checklist:
actionLocation
toactionRef
as suggested by @brasmussontest-run-starting
so that it could work forgherkin-lint
too