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

[wip] event-protocol: Add events & schemas for test runs #172

Closed
wants to merge 9 commits into from

Conversation

mattwynne
Copy link
Member

@mattwynne mattwynne commented Mar 26, 2017

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

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Change actionLocation to actionRef as suggested by @brasmusson
  • Deal properly with the location of steps from scenario outlines - include examples of all three types of steps:
    • steps from a regular gherkin scenario
    • steps from a scenario outline
    • steps from code (i.e. a hook)
  • find a better name for test-run-starting so that it could work for gherkin-lint too
  • resolve the difference between cucumber-ruby and gherkin about how exactly to model / name a location.

Copy link
Contributor

@aslakhellesoy aslakhellesoy left a 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.

@mattwynne
Copy link
Member Author

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 file_colon_line) for quite some time so I've become really used to it. I had spotted the discrepancy in language between cucumber-ruby and gherkin before, but this is the first time it's become clear as something we need to resolve.

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 file:line so much in the past.

The other puzzle I have around this is that we need to not only be able to use this to name the file:line of an element within a Gherkin document, but also a glue code method within a step definition file (what I've called actionLocation in this PR).

I guess we could just call those two attributes action and source and avoid naming the concept, but given that both of them will share the same structure, it makes sense to me to give it a name.

My instinct would to use the name location for the thing that includes the uri, and use the word position for the thing that's the line and maybe the column within the file. However I have a heavy bias from my current use of the word location, and I'm totally open to finding alternative names that work.

@brasmusson
Copy link
Contributor

Test Steps can be created from:

  • Gherkin steps from Scenarios - then they have exactly one sourceLocation
  • Gherkin steps from Scenario Outlines - then they have two sourceLocation:s (which is explicitly represented in Pickle Steps, in Cucumber-Ruby this is mostly hidden, but it does surface clearly in exception backtraces)
  • Hooks - then they have no sourceLocation

In case of Test Steps from Scenario Outlines, it would make sense to only represent the uri once, but defined two line:s.

In Ruby and other interpreted languages file:line does make sense when it comes to reference the action. In compiled languages you may not know the the source code file, or the line in it, where the action (step definition) is coded. For instance Cucumber-JVM use the class name and the method signature as the "location" of the action. A string attribute actionReference would be a more generally applicable definition, in case of interpreted languages that string could contain file:line, for other languages is could contain something different.

@mattwynne
Copy link
Member Author

@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 actionReference - that sounds sensible. Could you give me a concrete example of what that would look like for a Java step definition?

@brasmusson
Copy link
Contributor

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:

    "locations": [
      {
        "column": 3,
        "line": 8
      },
      {
        "column": 1,
        "line": 3
      }

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).

Could you give me a concrete example of what that would look like for a Java step definition?

This is a copy of the pretty formatter printout for a scenario of the java-calculator example in Cucumber-JVM:

  Scenario: Give correct change    # cucumber/examples/java/calculator/shopping.feature:3
    Given the following groceries: # ShoppingStepdefs.the_following_groceries(ShoppingStepdefs$Grocery>)
    When I pay 25                  # ShoppingStepdefs.i_pay(int)
    Then my change should be 4     # ShoppingStepdefs.my_change_should_be_(int)

@mattwynne
Copy link
Member Author

Maybe those two could be grouped in an environment event?

Yeah, I wonder if we should just rename that event to be a bit more broadly applicable than just a test-run. Thinking of the environment suggestion, some other things I'd like to collect in it, potentially, are:

  • the command that was run (so we might be able to re-run it)
  • the platform / runtime / VM info (node, ruby, JVM version etc.)
  • environment variables
  • the current user
  • machine IP address

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? job-started?

This prepares the way for keeping protocol formatters / emitters
together in the event-protocol folder too.
mattwynne added a commit that referenced this pull request May 15, 2017
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.
@mattwynne mattwynne changed the title event-protocol: Add events & schemas for test runs [wip] event-protocol: Add events & schemas for test runs May 15, 2017
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.
@mattwynne mattwynne force-pushed the event-protocol-test-results branch from 6ccb76e to 15f9e57 Compare May 15, 2017 20:46
@mattwynne mattwynne requested a review from l3pp4rd May 15, 2017 21:10
@mattwynne
Copy link
Member Author

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.

@l3pp4rd
Copy link
Member

l3pp4rd commented May 17, 2017

Concerning location and uri, is there any reason why we are not following the RFC for URI scheme like file:///home/user/project/features/my.feature that would be fully compatible for files and web resources. Though, that would change to absolute resource locations, but it may make sense in general. And if we decide to go one step further, for example even when running gherkin CLI such resource uris would allow to run features from various sources like:

  • web: http://example.com/features/my.feature
  • file: file:///home/user/project/features/my.feature or relative features/my.feature

In order to produce events.

@mattwynne
Copy link
Member Author

@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?

@l3pp4rd
Copy link
Member

l3pp4rd commented May 17, 2017

@mattwynne with relative paths I only meant that gherkin CLI tools can treat it as usual files as before, but
construct events with absolute file URI instead, that was it about it.

@mattwynne mattwynne force-pushed the event-protocol-test-results branch from 4daf199 to 24f7335 Compare June 24, 2017 14:03
@mattwynne
Copy link
Member Author

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?

@brasmusson
Copy link
Contributor

brasmusson commented Jun 30, 2017

I think that the test-step-started and test-step-finished should have its own source locations - the same as the pickle steps has (when the test step comes from a pickle step), and not only be defined relative to the the test case. When writing formatters based on event in Cucumber-JVM I have extensively use the ability to look up things in the source based on the locations references to the feature file.

In general I expect test-case event and test-steps event to be like the pickles and pickle steps - plus some additional information.

@mattwynne
Copy link
Member Author

Ok @brasmusson so we'd repeat the sourceLocation and actionRef (currently actionLocation but to be renamed) in each of the step events so you didn't have to listen for test-case-prepared unless you wanted to?

I'm fine with doing that 👍 as long as we also keep the pointer to it's position within the test case overall.

@charlierudolph
Copy link
Member

@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

  • test-step-attachment which has {testCase, index, data, media} used when attaching data to steps. I think the current attachment event should be renamed to source-attachment.
  • test-run-finished which has the structure {result: {success: true/false}} used to signal the finish of the run.

@brasmusson
Copy link
Contributor

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 before_test_case/after_test_case, before_test_step/after_test_step API methods, both the before and after methods received the TestCase/TestStep object, so everything that the before method could find out about the TestCase/TestStep, the after method could also find out. This has probably influenced by thinking to be more on the self contained side with respect of defining events.

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 test_run_started event, which could be useful for some listeners to know, but which assumes that all feature files are parsed and compiled before any test case is executed. This in turn blocks a design that can perform the execution of test cases from one feature file in parallel with parsing other feature files.

@stale
Copy link

stale bot commented Nov 8, 2017

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.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 8, 2017
@mattwynne mattwynne removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 8, 2017
@charlierudolph
Copy link
Member

@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.

@mattwynne
Copy link
Member Author

@charlierudolph I'd say we could merge this as-is and then iterate from there.

Next steps I think would be:

  • run checks for compliance with this protocol as part of the CI build for cucumber-js and cucumber-ruby's respective event plugins
  • resolve any inconsistencies we see in doing that

@stale
Copy link

stale bot commented Jan 27, 2018

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.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Jan 27, 2018
@aslakhellesoy aslakhellesoy added the 🧷 pinned Tells Stalebot not to close this issue label Jan 31, 2018
@stale stale bot removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Jan 31, 2018
@aslakhellesoy
Copy link
Contributor

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.

@aslakhellesoy aslakhellesoy deleted the event-protocol-test-results branch July 12, 2018 21:12
@lock
Copy link

lock bot commented Jul 12, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
library: cucumber-messages 🧷 pinned Tells Stalebot not to close this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants