Add test coverage

Can you post the patch here on the issue?

@juampynr, sorry for the delay, I'm going through this now. Is it common for modules to include circle ci test configuration? I've seen some modules add travis. Just curious as I've not seen it in other modules before.

@juampynr yeah, lets remove the circleci stuff from the patch for now. If we determine we want to use a testing platform for it we can then decide to add it back or go with travis.

Overall looks solid. We're near to RTBC great work.

1. +++ b/README.md
   @@ -0,0 +1,105 @@
   +[![CircleCI](https://circleci.com/gh/juampynr/adobe_analytics.svg?style=svg)](https://circleci.com/gh/juampynr/adobe_analytics)
   

   I'm fine with renaming it to Readme.md from the txt file. Please remove the CIRCLE CI information from it.

2. +++ b/adobe_analytics.module
   @@ -5,7 +5,7 @@
   +use Drupal\Core\Routing\RouteMatchInterface;
   

   😍

3. +++ b/adobe_analytics.module
   @@ -22,16 +22,16 @@ function adobe_analytics_help($route_name, RouteMatchInterface $route_match) {
   +  return [
   

   😍

4. +++ b/adobe_analytics.services.yml
   @@ -1,4 +1,4 @@
   +    arguments: ['@config.factory', '@current_route_match', '@current_user', '@module_handler', '@router.admin_context', '@token']
   

   I love this change but this service is public and any module using this service will break due to the arguments change.However, let's go with this change but mention on the release Notes in bold about this change

5. +++ b/composer.json
   @@ -1,6 +1,15 @@
   +        "behat/mink-extension": "v2.2",
   +        "cweagans/composer-patches": "^1.6",
   +        "behat/mink-selenium2-driver": "1.3.x-dev",
   +        "drupal/coder": "^8.2",
   +        "phpmd/phpmd": "^2.6",
   +        "phpmetrics/phpmetrics": "^2.3",
   +        "php-coveralls/php-coveralls": "v2.0.0"
   

   We don't need these dev requirements because drupal CI will take care of coding standards and coder

6. +++ b/patches.json
   @@ -0,0 +1,5 @@
   +{
   

   Let's remove this as well.

7. +++ b/src/AdobeAnalyticsHelper.php
   @@ -68,13 +76,27 @@ class AdobeAnalyticsHelper {
   +  public function __construct(ConfigFactory $config_factory, CurrentRouteMatch $currentRouteMatch, AccountProxyInterface $current_user, ModuleHandlerInterface $moduleHandler, AdminContext $admin_context, Token $token) {
   

   This change will be covered as part of the service definition change.
   Let's do it 😍

8. +++ b/src/AdobeAnalyticsHelper.php
   @@ -251,7 +269,7 @@ class AdobeAnalyticsHelper {
   +    $is_admin = $this->adminContext->isAdminRoute();
   

   😍

@juampynr I think that adding the field moves it out of the scope of the ticket. I think we can add that in, but wonder if we can do it in a separate task and patch. It makes it easier to review. Or can you at least post the interdiff so its easier to see the field related differences.

@juampynr actually no need, I am reviewing now and can review it all together.

I think the patch looks good. It works as expected and code looks good. The only thing I am thinking is that this be updated:

+    $properties['include_custom_variables'] = DataDefinition::create('boolean')
+      ->setLabel(t('Include custom variables'))
+      ->setRequired(TRUE);
+    $properties['include_main_codesnippet'] = DataDefinition::create('boolean')
+      ->setLabel(t('JavaScript Code'))
+      ->setRequired(TRUE);

I feel like this should be opt-in rather than opt-out with defaults set. I know its technically the same thing, but I am wonder if the text should read "Exclude custom variables from config." and "Exclude main Javascript code from config." and have default be FALSE. Technically its a minor thing, but from a user perspective, I think that it is a better way.

Just a discussion point.

Other than that I think it looks good.

After thinking about it more I think that we can just merge as is. I will get this merged today.

Committed, thanks juampynr!