Closed (fixed)
Project:
Drupal Coverage Core
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
22 Sep 2016 at 19:53 UTC
Updated:
12 Nov 2016 at 06:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
legovaerComment #4
legovaerPushed changes so far as creating automated tests has an increased priority at this point in time.
Comment #5
legovaerComment #6
jonathan1055 commentedJust had a browse at http://cgit.drupalcode.org/drupal_coverage_core/commit/?id=8da17eb

I am not familiar with the code at all, and have not run anything obviously, but I noticed one liitle change that you might have meant to do differently:
You have added
$machine_nameas the key in the foreach, but not actually used it. Did you mean to have$modules[$machine_name] = $module['name']instead?As I said, this is just from looking at some of the code. I have no idea exactly what all this is doing, but I just spotted that you must have made that change for a purpose.
Comment #7
legovaerJonathan,
Thanks for looking into this. The last commit was my work in progress on this issue. As more people are starting to work on this project, I had to switch to creating the unit tests for this project.
Your point is actually very valid and should get fixed.
In the meantime, I gave you access to the project on Acquia and I've updated the project page so that other people can ask for an invite for Acquia as well.
Comment #8
legovaerComment #9
legovaerFixed #6 and made sure that we can test D7 core modules.
Comment #11
legovaerComment #13
legovaerAdded the config factory to the argument of the
ModuleManagerservice.Comment #15
legovaerComment #16
bramdriesenLooks good! :) great job
Comment #17
jonathan1055 commentedReferring to my point back in #6 you say you fixed it in #9 but the original change still appears in the latest patch
Maybe you changed your mind and decided to leave it as-is, but just thought I would mention it, in case this got missed in your re-roll for 8.x-1.x
Comment #18
legovaerRight, I missed that! Thanks for pointing out!
Comment #19
bramdriesen#17 looks good, for the rest no changes since last time
Comment #21
legovaerThanks everyone!