Skip to content

Conversation

@msuperina
Copy link
Contributor

@msuperina msuperina commented Jul 24, 2018

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.

@msuperina msuperina self-assigned this Jul 24, 2018
@msuperina msuperina force-pushed the tests-driver-refactoring branch 2 times, most recently from 09a7ac6 to 501c376 Compare July 24, 2018 15:26
- npm prune
script:
- npm run build
- if [ "$TRAVIS_PULL_REQUEST" = "false" ]; then npm run test:ci; fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise this is an "in-progress" PR so you may be adding this back in later but wanted to highlight the following - https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions. I don't think makes any difference for this PR given that its scottlogic/finput:tests-driver-refactoring -> scottlogic/finput:tests-driver-refactoring (not a PR from an outside fork), but it will matter if this gets merged for PRs from other forks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the link you pointed is the reason why I am pushing directly to upstream :)

- if [ "$TRAVIS_PULL_REQUEST" = "false" ]; then npm run test:ci; fi
- npm run test
after_success:
- npm run semantic-release
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this is going to be added back in once the tests have been fixed and that you've removed this just so you can trigger builds/tests without performing a new release. Just wanted to flag that this is a required part of the build/release process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will revert the .travis.yml before the proper commit, I wanted to make sure my builds on this PR were not releasing anything...

@msuperina msuperina force-pushed the tests-driver-refactoring branch from 501c376 to acb3d72 Compare July 24, 2018 15:41
@msuperina
Copy link
Contributor Author

@ryanggrey I just created a commit without the modifications to the travis config

@msuperina msuperina requested a review from oriondean July 24, 2018 15:42

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` });
Copy link
Contributor

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.

@msuperina
Copy link
Contributor Author

Closing for #124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants