Desktop: Add test target to CircleCI config #13325
Conversation
The HAPPYCHAT_RECEIVE_MESSAGE event needed to filter out the meta.forOperator messages so they don't show to user
spen
added
Build
[Feature] WordPress Desktop App
spen
force-pushed
the
try/merge-wp-desktop-project--circle-ci
branch
from
2d66fab
to
937ae21
Compare
spen
force-pushed
the
try/merge-wp-desktop-project--circle-ci
branch
3 times, most recently
from
62ece26
to
7da57ac
Compare
spen
force-pushed
the
try/merge-wp-desktop-project--circle-ci
branch
from
7da57ac
to
33dd9e0
Compare
desktop/Makefile
Outdated
| @@ -158,13 +158,11 @@ node_modules/%: | |||
| node_modules: package.json | |||
| @$(NPM) prune | |||
| @$(NPM) install | |||
| @touch node_modules | |||
There was a problem hiding this comment.
I'm not so sure that this is the right solution - it avoids an error in the CircleCI build which I believe stems from an issue with node-touch. My worry here is that there is good reason for touching node_modules at the end of these make targets. @johngodley, it looks like these were added in the initial commit - are they safe to remove like this?
There was a problem hiding this comment.
Sorry I can't remember what the reason is. If it seems to work and nothing else has a problem then removing it is probably ok, and it's easy to add back if something does happen.
circle.yml
Outdated
| @@ -25,7 +25,7 @@ test: | |||
| - server/**/*.jsx | |||
| - MOCHA_FILE=./test-results-client.xml npm run test-client -- -R mocha-junit-reporter -t $CIRCLE_NODE_TOTAL -i $CIRCLE_NODE_INDEX: | |||
| parallel: true | |||
| - MOCHA_FILE=./test-results-client.xml npm run test-desktop -- -R mocha-junit-reporter -t $CIRCLE_NODE_TOTAL -i $CIRCLE_NODE_INDEX: | |||
| - npm run test-desktop: | |||
There was a problem hiding this comment.
I removed the extra arguments but this could be a problem - will we miss some reports by not including them?
There was a problem hiding this comment.
It takes almost 9 minutes to finish desktop test:
It is also executed on all 4 containers with the same tests, because parallel flag is enabled. More info about parallel can be found here: https://circleci.com/docs/1.0/parallel-manual-setup/.
spen
added
[Status] Needs Review
There was a problem hiding this comment.
It takes almost 9 minutes to finish desktop test:
It is also executed on all 4 containers with the same tests, because parallel flag is enabled. More info about parallel can be found here: https://circleci.com/docs/1.0/parallel-manual-setup/.
I don't think it is a good idea to increase build time by 9 minutes just to run one test. We should rather make sure that this check is part of desktop app deploy process.
spen
force-pushed
the
try/merge-wp-desktop-project
branch
from
a38ca9a
to
2b5586d
Compare
|
@spen This PR needs a rebase |
spen
force-pushed
the
try/merge-wp-desktop-project
branch
from
2b5586d
to
248c843
Compare
|
Closing as this PR is stale - please reopen if necessary |
matticbot
removed
the
[Status] Needs Review
As part of the
wp-desktop> Calypso ( #12430 ) I'm looking to foldwp-desktops CircleCI configuration in to that of Calypso.I'm quite new to CircleCI, particularly with configuration so this might not be the best approach - I'll be interested to hear if this approach is on the right track 😃
In PR #3793 there's mention of an issue with
wp-desktopbuilds preventing us from using all 8 containers. I'm not sure how relevant this information is now as this PR is over a year old but this could hopefully be another small win resulting from the merge 🎉For now, I'm opening this PR to see how CircleCI builds run with the new config so I've add a WIP label.