chore: enable cucumber, rewrite existing tests #513
Conversation
|
Deploy preview for dhis2-ui-core ready! Built with commit 4ad0a56 |
|
Lacks docs and proper PR description. |
There was a problem hiding this comment.
I can't really comment on the quality of this setup, because I don't know much about Gherkin at all. I have some generic comments:
Some file have some complex looking logic in them, i.e. cypress/assets/unfetch.umd.js and possibly this is something we'd need to copy into every repo that uses Gherkin. Perhaps we can use some form of package to take care of this? Or perhaps keep all these scripts in a central location (@dhis2/cli or sth)?
I've tested the cypress:open and cypress:run commands. The open command works fine, but the run command failed consistently (I've tried the built in terminal, iTerm, and the VSCode integrated terminal and they all had the same problem, quite likely this all boils down to the same thing as the all just use bash). Here's the output:
henkbookpro:ui hendrik$ yarn cypress:run
yarn run v1.17.3
$ node scripts/cypress_run.js
$ STORYBOOK_INCLUDE_TESTING=1 yarn start --ci
$ yarn start-storybook --ci
$ start-storybook --port 5000 -s ./stories/info --ci
webpack built c3b580e33c67785ba278 in 5527ms
╭────────────────────────────────────────────────────╮
│ │
│ Storybook 5.2.4 started │
│ 6.44 s for manager and 6.06 s for preview │
│ │
│ Local: http://localhost:5000/ │
│ On your network: http://192.168.87.189:5000/ │
│ │
╰────────────────────────────────────────────────────╯
$ NO_COLOR=1 cypress run -b $BROWSER --config video=false
Path must be a string. Received true
TypeError: Path must be a string. Received true
at assertPath (path.js:28:11)
at Object.basename (path.js:1395:5)
at isValidPathToBrowser (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/server/lib/browsers/index.js:52:17)
at /Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/server/lib/browsers/index.js:73:11
at tryCatcher (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/util.js:16:23)
at Promise._settlePromiseFromHandler (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/promise.js:512:31)
at Promise._settlePromise (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/promise.js:569:18)
at Promise._settlePromise0 (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/promise.js:614:10)
at Promise._settlePromises (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/promise.js:694:18)
at _drainQueueStep (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/async.js:138:12)
at _drainQueue (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/async.js:131:9)
at Async._drainQueues (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/async.js:147:5)
at Immediate.Async.drainQueues (/Users/hendrik/Library/Caches/Cypress/3.4.1/Cypress.app/Contents/Resources/app/packages/launcher/node_modules/bluebird/js/release/async.js:17:14)
at runCallback (timers.js:789:20)
at tryOnImmediate (timers.js:751:5)
at processImmediate [as _immediateCallback] (timers.js:722:5)
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Unfetch is only temporarily needed until cypress supports hooking into fetch. |
|
@Mohammer5 why do we need In cypress tests for applications we should be able to simply |
You're right, we don't need it in I'll remove it from this PR |
|
Nice! I think it also shouldn't be necessary in |
I'm using that to test different scenarios, like checking what happens when there are messages and when there are no messages. Setting the defaults It works quite well with gherkin as well, as there'd be nothing to be done by the |
cypress/fixtures/example.json
Outdated
| @@ -0,0 +1,5 @@ | |||
| { | |||
There was a problem hiding this comment.
Probably should remove this example fixture if we're not using it
There was a problem hiding this comment.
It looks like this file is still here?
There was a problem hiding this comment.
No idea what happened there.. It's gone now
There was a problem hiding this comment.
Yay tests, good work @Mohammer5 !
I made some comments inline.
One question - what is the reasoning behind everything in scripts/cypress? It seems like this could mostly be achieved in the package.json scripts with concurrently and wait-on - see maps package.json and maps .travis.yml - we don't use concurrently there but I think we could/should instead of starting the background process (see usage in app-runtime and app-store.
I'm also not sure what cypress:ci is for, why isn't that just cypress:run?
|
@amcgee Thanks for taking a look at this!
I've tried to use concurrently and wait-on. Unfortunately wait-on doesn't support waiting for a 200 response, so it'll continue earlier than when storybook is ready. (See "Goals") The scripts also make sure that the script fails if the port used by both cypress and storybook is identical, it exits with 1 if the port is already in use. Storybook just starts with an incremented port if the specified one is already in use and there's no way to know about that except for analyzing the output.
I guess the naming is a bit odd here, I'd appreciate alternative names that are more descriptive! |
Do we need to wait for a |
I haven't tried |
Just throwing out possibly-simpler solutions here, since I'm worried that adding a bunch of custom development scripts to a single repo (though I know we'll extract them into a wrapper lib eventually): Could we use detect-port's CLI to either fail if the port isn't available or pass a random available port to Storybook and on to Cypress? |
Found that after a lot of frustration and debugging with this same issue in maps, it's handy! |
|
Thanks to @amcgee I was able to get rid of the scripts folder and do everything with concurrently and wait-on 👍. |
|
LGTM. As I mentioned in the other Cypress/Cucumber PR: I don't have experience setting it up so basing my review on how it looks and works. If there are any improvements to be made they can be done incrementally. |
|
Would be great if you could take another look @amcgee. |
There was a problem hiding this comment.
LGTM (after removing the unused feature again, minor comments below)
I can't say I'm crazy about the sentence-length file/directory names (really easy to mis-match them, since they need to be identical), though I can live with that here.
Also not a deal-breaker, but here's a suggestion for script names that make them a little more descriptive -
cypress:run // starts the server and runs cypress:only:run
cypress:open // starts the server and runs cypress:only:open
cypress:only:run // cypress run
cypress:only:open // cypress open
Not sure I love these either, but at least it's easier to parse than the difference between cypress:open and cypress:browser.
We could make these CLI flags in the eventual wrapper library, so we wouldn't need ugly long script names.
cypress/fixtures/example.json
Outdated
| @@ -0,0 +1,5 @@ | |||
| { | |||
There was a problem hiding this comment.
It looks like this file is still here?
| }, | ||
| "projectId": "65oh1u" | ||
| "testFiles": "**/*.feature", | ||
| "video": false, |
There was a problem hiding this comment.
Why are we setting video: false here? Seems like we want videos when doing recorded CI runs...
There was a problem hiding this comment.
Same reason as for the screenshots: I think this should be explicitly turned on for CI. During development this won't be needed as we're probably debugging in the browser anyways.
These files will be git-ignored, so on the one hand devs won't necessarily notice these files have been created and on the other hand is the amount of disk space used by videos quite a bit.
There was a problem hiding this comment.
Ah you're right, totally fair! I'd used the --video false flag for that, but putting it in the config file is definitely better if we can override it explicitly when running in CI. Nice.
There was a problem hiding this comment.
Ok.. seems like right now that's not an option: kimmobrunfeldt/concurrently#33
So I'm inclined to remove these options from cypress.json until this 3.5 year old issue has been solved..... Do you agree?
There was a problem hiding this comment.
https://docs.cypress.io/guides/guides/environment-variables.html#Setting
Looks like an env bar would work too?
There was a problem hiding this comment.
It's a bit more complicated. We'd have to set the config with code, depending on an env var, and I wasn't successful in doing so (I didn't try extensively though).
I think for now it's ok to remove it from the config and do this on a new/another branch/PR
There was a problem hiding this comment.
This works (with "video": false in cypress.json)
CYPRESS_VIDEO=true cypress run
I think you're right, it's better not to create screenshots or videos on a dev machine so let's default those to false in cypress.json (as you had done) and override them with the env vars in .travis.yml
There was a problem hiding this comment.
(no configuration with code required)
There was a problem hiding this comment.
^ @Mohammer5 (and yeah, not sure about the Semantic status check, think @varl might have changed the settings for this repo to require all commits to be semantic, since this is merge-rebased?)
There was a problem hiding this comment.
I've added this functionality for taking screenshots manually as that didn't work.
Works for videos out of the box.
Mohammer5
changed the title
Mohammer5
force-pushed
the
Enable_cucumber
branch
from
6696d77
to
cd1c28b
Compare
This PR covers several topics
Cucumber
I've added the
cypress-cucumber-preprocessornpm module.This allows us to write
.featurefiles in thecypress/integrationfolder.I've set
"nonGlobalStepDefinitions": truein thepackage.jsonto enable us to place the step definitions in folders with the same name as the.featurefileRewrite existing tests
I've rewritten the previously existing tests for the AlertBar.
There are now two feature files in
cypress/integration:AlertBar/Autohiding.featureStep definitions in ``cypress/integration/AlertBar/Autohiding/`
AlertBar/ClearOnAction.featureStep definitions in ``cypress/integration/AlertBar/ClearOnAction/`
Add scripts
I've added 5 scripts to the package.json of which 2 are of importance to us:
cypress:opencypress:runThese will both run the storybook and then cyrpress.
What to check here:
(When running
yarn cypress:run, make sure your BROWSER env var contains the path to either chrome or chromium:BROWSER=$(which chrome) yarn cypress:run)yarn cypress:openshould simply open the cypress interfaceyarn cypress:runshould run the tests inside the terminalI've tested it on a different tty to ensure it's working properly because in ttys with a window manager, it still opens a real browser
cypress:openprocess is terminated manually:There should be no running process of the storybook or cypress
cypress:runshould exit with code 0 if all tests passThe script should exit with code 1
The
cypress:runscript should exit with code 1