Enhanced the provisioning to better adhere to VVV2 standards #2

jcomack · 2017-10-31T15:39:28Z

This PR introduces a new provisioning script that better adheres the standards of VVV2.

By using this script, the process should also better reflect Yoast standards.

To test, please create a new virtual environment by executing the steps described in README.md and ensure you can run both the PHP and the JavaScript tests.

…users

kraftner

First one general thing about the approach: Why did you replace Composer with using WP-CLI for installing WP? I think this makes things way more difficult to read.

I haven't actually tried this, this is just a first look at the changes, will try to do so later this week.

kraftner · 2017-10-31T15:58:21Z

README.md

        repo: https://github.com/kraftner/yoast-acf-analysis-development.git
        hosts:
-            - yoast-acf-analysis.vvv.dev
+            - yoast-acf.dev


Not sure about the naming:

We shouldn't use .dev but .testinstead. See Change TLD for test sites Varying-Vagrant-Vagrants/VVV#583 for details

To stay in line with Rename repository to match the WordPress.org slug Yoast/yoast-acf-analysis#99 I'd say we should call it acf-content-analysis-for-yoast-seo.vvv.test or at least yoast-acf-analysis.vvv.test for now but not a new name again.

kraftner · 2017-10-31T16:01:38Z

README.md

@@ -36,7 +40,10 @@ If you want to auto-configure your VVV machine with those add the following to y

 After that to run the tests for ACF 4 run `npm run test-acf4` inside the plugin folder.

-To run the tests for ACF 5 you first need to install ACF 5 as we cannot do this automatically as it isn't a free plugin.
-After you've done that you can simply run `npm run test-acf5pro`.
+To run tests for ACF 5 you first need to install ACF 5 manually, as we cannot do this automatically due to it not being a free plugin.


This should now be ACF 5 PRO as the release of ACF 5 FREE is imminent.

kraftner · 2017-10-31T16:04:06Z

README.md

+After you've installed ACF 5, you can simply run `npm run test-acf5pro`.
+
+**Note:**
+If you do not want to run the browser tests with Chrome and/or in headless mode, you'll need to change `nightwatch.json`, `nightwatch.conf.example.js` and `env.js` in your virtual machine. 


This was intentionally written using some and not and as depending on what you are trying to do only some need to be changed.

This is the relevant docs, maybe a link would be nice though: http://nightwatchjs.org/gettingstarted#settings-file

kraftner · 2017-10-31T16:10:11Z

provision/.env.js

-        var page = browser.page.WordPressHelper();
-        page.login();
-    }
-};


I need to check back on this but there was a particular reason why I was doing it like this...I just can't remember now. Should have documented that...

When I attempted to use the previous configuration, I couldn't get the tests to run at all. @moorscode found that basic configuration which did seem to do the trick, hence why I changed it.

Ah I remember now what the issue was: When using the plain chromedriver without xvfb the headless chrome wasn't properly working on any interaction with an input (something about missing keycode conversion without a window manager).
Did you try provisioning with that change and run the Nightwatch tests successfully? Maybe they have meanwhile fixed this in a recent Chrome version.

The tests weren't successful as logging in resulted in an immediate redirect to the login page. I don't believe that's related to this change.

Okay I've looked into this and it seems as if Chrome fixed this in the meantime, so yeah, this change is ok by now.

kraftner · 2017-10-31T16:10:54Z

provision/vvv-hosts

Okay here is .test like proposed above. This should be the same obviously.

jcomack · 2017-11-01T09:05:14Z

The main reason I removed the Composer package that handles the installation of WordPress, is because VVV had a perfectly fine (and flexible) method to install WordPress for us. See https://github.com/Varying-Vagrant-Vagrants/custom-site-template for the template that I used for this new setup.

Keeping all installations similar in file structure makes it easier to work with this for multiple people. That was my main thought behind all this :)

kraftner · 2017-11-02T10:14:21Z

First things first: Thanks for the changes, the majority of this looks great!

Concerning the switch away from Composer: Don't get me wrong - I totally understand your reasoning and appreciate the intention to make everything more approachable.

The thing is that the same reasoning goes the other way for me: I build everything WP I do using Composer and WP Starter nowadays so for me personally this complicates things as I need to mentally adapt to another stack. I'm currently pondering if I should just live with that or a fork might be the better option. I'll get back to you on this soon. 😄

tomjn · 2017-11-03T15:25:15Z

I would recommend using WP CLI if a WP root package is necessary, else composer might delete WP and replace it, deleting all the nested packages in the process. It also makes it easier to pass in variables from the provisioner script

tomjn · 2017-11-03T15:27:21Z

provision/vvv-hosts

this file isn't necessary if you've already got the hosts in vvv-custom.yml, support for vvv-hosts is mostly for convenience and backwards compatibility with VVV 1

kraftner · 2017-11-03T15:29:06Z

@tomjn 👋 How did you end up here 😉
If by root package you mean not having core in a separate folder that is a no-brainer when using composer anyway.
The variables argument is a valid one though.

kraftner

Okay I have now finally found time to look into this, thanks for your patience. I am still not totally happy about the switch away from wpstarter/composer but I understand the reasoning to keep it a more standard setup to make it easier to grasp the setup for contributors. So I am willing to merge this in once we have sorted out the issues below.

Concerning the issues you had with running the JS tests I was able to reproduce it and have found the reason:
With the current setup WP Core is in a subfolder, so

http://example.com/wp/wp-admin

which I (unfortunately 😞 ) also hardcoded in the tests here and here

By changing the install to a more default one the extra /wp/ is triggering a logout because of the invalid address (Don't ask me why that is, for some reason WP seems to think it is a smart idea to log out people after opening an invalid admin URL 🙄)

Now, to fix this we need to change this in the main plugin repo which on the other hand breaks all the tests in the current setup. Although this is a breaking change it will only affect people working on the plugin so I think we should do it. But at least do it simultaneously as soon as this PR is ready to be merged in.

kraftner · 2017-12-07T10:47:31Z

README.md

-            - yoast-acf-analysis.vvv.dev
+            - acf-content-analysis-for-yoast-seo.test
+
+**Note that you can change `acf-content-analysis-for-yoast-seo.test` to any other name, if that's what you prefer.**

 If you do not want to provision the default sites that come with VVV make sure to *only* have this in the `site` section.


It should say sites here

kraftner · 2017-12-07T10:52:39Z

provision/vvv-init.sh

+  sed -i "s#{{DOMAIN}}#http://${DOMAINS}#" "${VVV_PATH_TO_SITE}/public_html/wp-content/plugins/yoast-acf-analysis/tests/js/system/nightwatch.conf.js"
+  echo "Configuring complete"
+
+  noroot wp plugin activate --all


By installing WP without composer we now have "Hello Dolly" and "Akismet" installed. 😒
So I'd either replace this with

noroot wp plugin activate advanced-custom-fields wordpress-seo yoast-acf-analysis

or add a

noroot wp plugin delete hello akismet

before this line.

kraftner · 2017-12-07T11:08:42Z

provision/vvv-init.sh

+  cp ${VVV_PATH_TO_SITE}/provision/.env.js ${VVV_PATH_TO_SITE}/public_html/wp-content/plugins/yoast-acf-analysis/tests/js/system/.env.js
+  cp ${VVV_PATH_TO_SITE}/public_html/wp-content/plugins/yoast-acf-analysis/tests/js/system/nightwatch.conf.example.js ${VVV_PATH_TO_SITE}/public_html/wp-content/plugins/yoast-acf-analysis/tests/js/system/nightwatch.conf.js
+
+  sed -i "s#{{DOMAIN}}#http://${DOMAINS}#" "${VVV_PATH_TO_SITE}/public_html/wp-content/plugins/yoast-acf-analysis/tests/js/system/nightwatch.conf.js"


I think we should also automatically do

cd ${VVV_PATH_TO_SITE}/public_html/wp-content/plugins/yoast-acf-analysis composer install npm install

instead of adding this note. Without that the plugin won't init anyway because of the lack of the vendor folder.

kraftner · 2017-12-07T15:33:49Z

provision/vvv-init.sh

+if [[ ! -f "${VVV_PATH_TO_SITE}/public_html/wp-config.php" ]]; then
+  echo "Configuring WordPress Stable..."
+  noroot wp core config --dbname="${DB_NAME}" --dbuser=wp --dbpass=wp --quiet --extra-php <<PHP
+define( 'WP_DEBUG', true );


Lacks

define( 'AC_YOAST_ACF_ANALYSIS_ENVIRONMENT', 'development' );

without which the testing data won't get loaded.

Jimmy Comack added 17 commits October 30, 2017 13:34

VVV2 overhaul

b2f07ef

Potential fix

1cb6734

Added missing lines

21ff228

Removed unnecessary line

5a1825b

Added wp-cli template

1f2c844

Attempt to remove installation of WordPress via Composer

f118216

Moved some commands and added feedback

75b6f16

Added test

eb6dd73

Fixed config

45ae7ae

Added additional replace var and updated .env.js

91731c7

Added quiet option to composer

c046ca4

Added missing http prefix in replace var

aaf74c4

Using Yarn instead of NPM

a0dcba3

Another attempt at getting NPM to work

05e599f

Removed composer and npm install as this should be an action done by …

831e835

…users

Updated readme

bb5cb14

Tweaked documentation

e1534fa

kraftner requested changes Oct 31, 2017

View reviewed changes

Processed CR

62cb471

tomjn reviewed Nov 3, 2017

View reviewed changes

Removed legacy vvv-hosts

ee8ef40

kraftner approved these changes Dec 7, 2017

View reviewed changes

kraftner requested changes Dec 7, 2017

View reviewed changes

kraftner reviewed Dec 7, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced the provisioning to better adhere to VVV2 standards #2

Enhanced the provisioning to better adhere to VVV2 standards #2

jcomack commented Oct 31, 2017

kraftner left a comment •

edited

Loading

kraftner Oct 31, 2017

kraftner Oct 31, 2017

kraftner Oct 31, 2017

kraftner Oct 31, 2017

jcomack Nov 1, 2017

kraftner Nov 3, 2017

jcomack Nov 6, 2017

kraftner Dec 7, 2017

kraftner Oct 31, 2017

jcomack commented Nov 1, 2017

kraftner commented Nov 2, 2017

tomjn commented Nov 3, 2017

tomjn Nov 3, 2017

kraftner commented Nov 3, 2017

kraftner left a comment

kraftner Dec 7, 2017

kraftner Dec 7, 2017

kraftner Dec 7, 2017

kraftner Dec 7, 2017

Enhanced the provisioning to better adhere to VVV2 standards #2

Are you sure you want to change the base?

Enhanced the provisioning to better adhere to VVV2 standards #2

Conversation

jcomack commented Oct 31, 2017

kraftner left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcomack commented Nov 1, 2017

kraftner commented Nov 2, 2017

tomjn commented Nov 3, 2017

Choose a reason for hiding this comment

kraftner commented Nov 3, 2017

kraftner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraftner left a comment •

edited

Loading