Provides art command #1191

dustinleblanc · 2016-09-13T17:41:36Z

Still need to write proper test cases. but the change to the ./bin/terminus.php is pretty important, so we may want to get this merged.

to use the art command, just run ./bin/terminus.php art rocket and enjoy

pantheon-mentionbot · 2016-09-13T17:41:38Z

@dustinleblanc, thanks for your PR! By analyzing the blame information on this pull request, we identified @greg-1-anderson, @TeslaDethray and @bensheldon to be potential reviewers

coveralls · 2016-09-13T18:01:22Z

Coverage remained the same at 13.982% when pulling 2d4e1bd on dustinleblanc:artify into d57ad1e on pantheon-systems:master.

coveralls · 2016-09-13T18:06:10Z

Coverage remained the same at 13.982% when pulling b3d200c on dustinleblanc:artify into d57ad1e on pantheon-systems:master.

greg-1-anderson · 2016-09-13T18:09:46Z

bin/terminus.php

@@ -28,8 +28,7 @@
 $container = new Container();
 $input = new ArgvInput($_SERVER['argv']);
 $output = new ConsoleOutput();
-$roboConfig = new \Robo\Config(); // TODO: make Terminus Config extend \Robo\Config and use $config here
-Robo::configureContainer($container, $roboConfig, $input, $output, $application);
+Robo::configureContainer($container, $config, $input, $output, $application);


I guess I forgot to do this in my previous PR. Good catch.

TeslaDethray · 2016-09-13T18:12:58Z

@dustinleblanc Please open a separate PR for the change to bin/terminus.php.

coveralls · 2016-09-13T18:13:48Z

Coverage remained the same at 13.982% when pulling b3d200c on dustinleblanc:artify into d57ad1e on pantheon-systems:master.

dustinleblanc · 2016-09-13T18:17:29Z

Done, see #1192

dustinleblanc · 2016-09-13T18:19:23Z

I <3 git cherry-pick sha

TeslaDethray · 2016-09-13T18:20:28Z

Thank you for doing that so promptly, @dustinleblanc. I didn't realize that your base branch is out-of-date, though.
https://github.com/pantheon-systems/terminus/blob/master/bin/terminus.php

dustinleblanc · 2016-09-13T18:24:14Z

shouldn't be, it was rebased, and the current version still looks to be injecting $roboConfig which doesn't instantiate our constants. I need it to inject our config so that I can get the asset path

coveralls · 2016-09-13T18:37:36Z

Coverage remained the same at 14.05% when pulling 752cbdf on dustinleblanc:artify into 0f26f81 on pantheon-systems:master.

TeslaDethray · 2016-09-13T18:50:16Z

#1192/#1193 is merged.

dustinleblanc · 2016-09-13T18:50:52Z

@TeslaDethray 👍 I'll rebase this, get testing finished, and re-submit

coveralls · 2016-09-13T19:07:25Z

Coverage remained the same at 14.05% when pulling 34a293f on dustinleblanc:artify into 3c6909f on pantheon-systems:master.

greg-1-anderson · 2016-09-14T15:37:06Z

composer.json

@@ -24,7 +24,8 @@
  "autoload": {
    "psr-4": {
      "Terminus\\": ["php/", "php/Terminus/"],
-      "Pantheon\\Terminus\\": "src/"
+      "Pantheon\\Terminus\\": "src/",
+      "Pantheon\\Terminus\\Tests\\": "tests/"


I usually prefer to make this tests/src so that your test classes are not mixed in with your other testing files. Individual project variation is fine here, though -- up to @TeslaDethray

👍 I'm actually rebasing to cleanup right now

Reading further, it seems you only added this for tests/CommandTestCase.php. It should not be necessary to add an entry to the autoloader for this file (which I do think belongs immediately inside tests), as phpunit should already be loading it. Um, mistake, sorry, that's wrong. This is necessary.

When adding tests directory to the autoloader, you should put them in autoload-dev rather than autoload.

Sorry, misread your code the first time. Corrected my comment, you were right. Just move the directive to autoload-dev and it's perfect.

greg-1-anderson · 2016-09-14T15:47:29Z

Great work -- love the test class!

dustinleblanc · 2016-09-14T15:48:28Z

Thanks! I'm hoping it makes it easier for us to run faster tests that can load up a whole command, tip to tail and assert against it easily.

coveralls · 2016-09-14T15:49:10Z

Coverage remained the same at 14.05% when pulling 52d9ccf on dustinleblanc:artify into 3c6909f on pantheon-systems:master.

coveralls · 2016-09-14T15:49:10Z

Coverage remained the same at 14.05% when pulling 52d9ccf on dustinleblanc:artify into 3c6909f on pantheon-systems:master.

coveralls · 2016-09-14T16:00:50Z

Coverage remained the same at 14.05% when pulling c701a8f on dustinleblanc:artify into 3c6909f on pantheon-systems:master.

dustinleblanc · 2016-09-14T16:08:34Z

@TeslaDethray @greg-1-anderson Something that just occurred to me, though PHPUnit is running these, and they are good n snappy, they are functional tests rather than true unit tests. Should they be separated?

TeslaDethray · 2016-09-14T16:17:07Z

@dustinleblanc I was hoping that the protected functions, retrieveArt and and validate asset, would be tested individually, like @ajbarry has done in the connection:info PR.

dustinleblanc · 2016-09-14T16:17:57Z

@TeslaDethray sure! That will certainly increase our test coverage :)

coveralls · 2016-09-14T16:20:12Z

Coverage remained the same at 14.05% when pulling 2faaf27 on dustinleblanc:artify into 3c6909f on pantheon-systems:master.

greg-1-anderson · 2016-09-14T16:22:17Z

I often use phpunit to write functional tests. I don't always separate these from unit tests, although some projects do.

If separated, all of the tests should run in a single execution of phpunit, so that our test coverage can be calculated accurately.

dustinleblanc · 2016-09-14T16:26:29Z

@TeslaDethray oh yeah, I just realized that will require the reflection dance...I'll back burner it for now and perhaps wait for that to settle and just integrate whatever helpers come out of that if we do indeed want to test protected/private members.

greg-1-anderson · 2016-09-14T16:38:12Z

I think the general procedure should be:

Test the public methods.
Can I increase coverage by using reflection to test protected methods?
Can I increase coverage by using a test double and asserting that a method was called?

Avoid 2. and 3. unless necessary.

ajbarry · 2016-09-14T17:00:27Z

@TeslaDethray so our approach to the functional tests is asserting on the vcr replay, is that correct?

coveralls · 2016-09-14T18:50:38Z

Coverage remained the same at 14.05% when pulling d5e986d on dustinleblanc:artify into 859fce3 on pantheon-systems:master.

coveralls · 2016-09-14T19:56:05Z

Coverage remained the same at 14.05% when pulling 3e47e37 on dustinleblanc:artify into 3584938 on pantheon-systems:master.

coveralls · 2016-09-14T19:56:05Z

Coverage remained the same at 14.05% when pulling 3e47e37 on dustinleblanc:artify into 3584938 on pantheon-systems:master.

coveralls · 2016-09-14T19:56:05Z

Coverage remained the same at 14.05% when pulling 3e47e37 on dustinleblanc:artify into 3584938 on pantheon-systems:master.

TeslaDethray · 2016-09-14T20:37:53Z

Good work! I'm merging this in to build on.

dustinleblanc force-pushed the artify branch from cc5efab to b3d200c Compare September 13, 2016 17:57

greg-1-anderson reviewed Sep 13, 2016
View reviewed changes

dustinleblanc force-pushed the artify branch from b3d200c to 752cbdf Compare September 13, 2016 18:25

dustinleblanc force-pushed the artify branch from 752cbdf to 34a293f Compare September 13, 2016 18:52

dustinleblanc changed the title ~~[WIP] Provides art command~~ Provides art command Sep 14, 2016

greg-1-anderson reviewed Sep 14, 2016
View reviewed changes

dustinleblanc force-pushed the artify branch from 52d9ccf to c701a8f Compare September 14, 2016 15:47

dustinleblanc force-pushed the artify branch from c701a8f to 2faaf27 Compare September 14, 2016 16:06

dustinleblanc force-pushed the artify branch from 2faaf27 to d5e986d Compare September 14, 2016 18:38

dustinleblanc added 3 commits September 14, 2016 15:38

Shows art

6325c77

Add passing unit tests

9d81542

Add Command test helper

3e47e37

dustinleblanc force-pushed the artify branch from d5e986d to 3e47e37 Compare September 14, 2016 19:38

dustinleblanc mentioned this pull request Sep 14, 2016

Turning on VCR for testing #1200

Merged

TeslaDethray merged commit da4f14b into pantheon-systems:master Sep 14, 2016

Provides art command #1191

Provides art command #1191

Conversation

dustinleblanc commented Sep 13, 2016

pantheon-mentionbot commented Sep 13, 2016

coveralls commented Sep 13, 2016 • edited

coveralls commented Sep 13, 2016 • edited

greg-1-anderson Sep 13, 2016

Choose a reason for hiding this comment

TeslaDethray commented Sep 13, 2016

coveralls commented Sep 13, 2016 • edited

dustinleblanc commented Sep 13, 2016

dustinleblanc commented Sep 13, 2016

TeslaDethray commented Sep 13, 2016

dustinleblanc commented Sep 13, 2016

coveralls commented Sep 13, 2016 • edited

TeslaDethray commented Sep 13, 2016

dustinleblanc commented Sep 13, 2016

coveralls commented Sep 13, 2016 • edited

greg-1-anderson Sep 14, 2016 • edited

Choose a reason for hiding this comment

dustinleblanc Sep 14, 2016

Choose a reason for hiding this comment

greg-1-anderson Sep 14, 2016 • edited

Choose a reason for hiding this comment

greg-1-anderson Sep 14, 2016

Choose a reason for hiding this comment

greg-1-anderson commented Sep 14, 2016

dustinleblanc commented Sep 14, 2016

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016 • edited

coveralls commented Sep 14, 2016 • edited

dustinleblanc commented Sep 14, 2016

TeslaDethray commented Sep 14, 2016

dustinleblanc commented Sep 14, 2016

coveralls commented Sep 14, 2016 • edited

greg-1-anderson commented Sep 14, 2016

dustinleblanc commented Sep 14, 2016

greg-1-anderson commented Sep 14, 2016

ajbarry commented Sep 14, 2016 via email

coveralls commented Sep 14, 2016 • edited

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

TeslaDethray commented Sep 14, 2016

coveralls commented Sep 13, 2016 •

edited

coveralls commented Sep 13, 2016 •

edited

coveralls commented Sep 13, 2016 •

edited

coveralls commented Sep 13, 2016 •

edited

coveralls commented Sep 13, 2016 •

edited

greg-1-anderson Sep 14, 2016 •

edited

greg-1-anderson Sep 14, 2016 •

edited

coveralls commented Sep 14, 2016 •

edited

coveralls commented Sep 14, 2016 •

edited

coveralls commented Sep 14, 2016 •

edited

coveralls commented Sep 14, 2016 •

edited