Use cucumber-expressions to generate the match in fake-cucumber #793
Conversation
Note: there should be a better way to import CucumberExpression and Messages, still WIP
Makefile
Outdated
| cucumber-query/Makefile \ | ||
| cucumber-react/Makefile \ | ||
| html-formatter/Makefile \ | ||
| json-formatter/Makefile \ | ||
| datatable/Makefile \ | ||
| config/Makefile \ | ||
| cucumber-expressions/Makefile \ | ||
| fake-cucumber/Makefile \ |
There was a problem hiding this comment.
The dependency order is now wrong because cucumber-query’s tests depends on fake-cucumber.
Better to move cucumber-expressions up and leave fake-cucumber where it was.
There was a problem hiding this comment.
indeed, and the formatters also depend on this library. I fix that in the next commit.
Add a test to ensure the type given by cucumber-expression is shown in messages
|
Open question: previously, The |
There was a problem hiding this comment.
This is great - we'll be able to produce more faithful output using this, and I think that will be useful when we use fake-cucumber to validate/test consumers.
I do wonder if it might be worthwhile implementing the step definition and matching logic as if this were a real cucumber implementation. If we do that, we could evolve this to become a new codebase for Cucumber.js, with a simpler architecture and less baggage than the original.
| } from 'cucumber-expressions' | ||
| import uuidv4 from 'uuid/v4' | ||
|
|
||
| import Argument from 'cucumber-expressions/dist/src/Argument' |
There was a problem hiding this comment.
Don't reach inside a package for imports. This should be:
import { Argument } from 'cucumber-expressions'Which will require a change to cucumber-expressions
| import uuidv4 from 'uuid/v4' | ||
|
|
||
| import Argument from 'cucumber-expressions/dist/src/Argument' | ||
| import CucumberExpression from 'cucumber-expressions/dist/src/CucumberExpression' |
There was a problem hiding this comment.
the thing for this one is that, when I use import { CucumberExpression } from cucumber-expression, I can not use CucumberExpressionas a type for the variables, so I have to write weird stuff likepublic expression: any = new CucumberExpression` (which is pretty ugly too)
| import CucumberExpression from 'cucumber-expressions/dist/src/CucumberExpression' | ||
|
|
||
| class FakeStepDefinition { | ||
| public id: string |
There was a problem hiding this comment.
Try to make fields private readonly as much as possible to prevent exposing more internals than necessary
| public status: string, | ||
| public pattern: string = null | ||
| ) { | ||
| this.id = uuidv4() |
There was a problem hiding this comment.
Initialisation should be done at declaration
| if (pattern === null) { | ||
| this.pattern = `{}${status}{}` | ||
| } | ||
| this.expression = new CucumberExpression(this.pattern, new ParameterTypeRegistry) |
There was a problem hiding this comment.
Use parenthesis when calling constructors:
new ParameterTypeRegistry()I also think it is cleaner to pass the whole expression object through the constructor, which should declare the type as Expression (imported from cucumber-expressions). That way we could also pass in a RegularExpression if we want, and all the expressions can share the same ParameterTypeRegistry instance (since it's created outside)
| }) | ||
| }) | ||
| ) | ||
| this.stepDefinitions.makeMessages().forEach(message => { |
There was a problem hiding this comment.
Adding the step definitions here is problematic, since it will happen for each feature file. We don't want to do this for each feature file - we only want to do it once.
Maybe it's better to pass all the step definitions to the stream's constructor.
| const fakeMatch = new FakeMatch() | ||
|
|
||
| this.stepDefinitions.forEach(stepDefinition => { | ||
| const match = stepDefinition.expression.match(step) |
There was a problem hiding this comment.
Let's avoid train wrecks.
| const match = stepDefinition.expression.match(step) | ||
| if (match !== null) { | ||
| fakeMatch.stepMatchArguments = match | ||
| fakeMatch.stepDefinitionId = stepDefinition.id |
There was a problem hiding this comment.
Let's avoid setters too. They are a common source of bugs. It's better to make this field private readonly and make sure it's instantiated in the constructor.
We should use the parameter type name reported by cucumber-expressions (which will be |
…tus from the execution Also do some renaming after Match became SupportCodeExecutor Code refactoring
…nslation to protobuf
…ucumber/cucumber into fake-cucumber-use-cucumber-expressions
Summary
We've added default step definitions in the messages generated by
fake-cucumberrecently, but the choice of the step definition used for a TestStep was random.Now it uses
cucumber-expressionto make real match and generate thetestStepMatchArgumentmessages.Still TODO:
ambiguousstatuses.Details
Motivation and Context
Types of changes
Checklist: