-
Notifications
You must be signed in to change notification settings - Fork 11
chore: refactor specs to make it functions and expectations explicit #124
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
Conversation
|
|
||
| cutting(4).characters().from(`123456`).startingFrom(1).shouldShow(`156.00`); | ||
| cutting(5).characters().from(`1234`).startingFrom(0).shouldShow(``); | ||
| itCutting({ cut: 0, startingFrom: 0, tested: `123456`, expected: `123,456` }); |
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 refactor means we lose the bdd style syntax. The prior read as "cutting zero characters from '123456' starting from 0 should show '123,456'" and is very easy to understand the intent of the test. Whereas the new syntax requires knowing about the itCutting function and how the params are used. Is it not possible to keep the bdd type syntax by having the driver passed as a param to the customCommandsFactory? Perhaps I'm missing some other consideration that means this isn't possible.
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.
I do agree the previous syntax was better, although it was coupling the it call to the function passed to the it. And you still needed to know about cutting, characters, from, startingFrom, shouldShow, etc... Also, the main and perhaps real disadvantage was that the cutting wrapper needed to be defined statically.
The point of this commit is to have an expectation function separate from the it call. The expectation function is defined in the beforeAll. the itCutting is cumbersome, I agree, and I can remove it and add the text explicitely here, but at least it has the advantage to provide easy composition. Also, the api about the scenario under test is quite explicit.
Any thoughts ?
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.
It is my understanding that we want to be able to inject driver into each spec while being able to load before all specs and unload after all specs. I see your point about decoupling expect from it and how the proposed change uses that to inject the driver into the cutting function meaning each spec can use a different driver in the future.
I was wondering though if its possible to achieve this without changing the bdd style syntax. If we only want to be able to set the driver per description block then can driver just be injected as a param into the pre-existing customCommandsFactory? If we want to be able to set driver at the per spec level then would it be possible to add something like a usingDriver function to chainFunctions within the custom command that sets the driver for that spec (giving something like cutting(...).characters().usingDriver(...)...)? Perhaps I'm missing some consideration that means this wouldn't work though?
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.
ok let me try to come up with something, I will open another PR
|
Closing after discussion in favor of #125 |
Refactoring of how the custom commands are consumed by the test specs.
Each spec need to get a driver, load it before all the tests and unload it after all the tests.
customCommands now exports functions to be composed.
A set of functions to run the expectation in the it code, and a set of helpers to automate the writing of the it functions.
Note: although all the specs need to get a driver, there is only one instance of driver shared by all the specs (tests are run in sequence). This can change - and it will be easy to change the code with this refactoring - but so far I have had timeout issues when creating many instances, and have not figured out what is the exact issue.