Success

User tests: Successful: Unsuccessful:

avatar elkuku
elkuku
23 Jul 2016

Summary of Changes

  • Refactor JoomlacmsPullsListener::checkFilesAndAssignCategory()
  • Add a unit test for this method

Testing Instructions

Check if all files are correct for the corresponding category.
I believe that there are some inconsistencies.
Have a look at the unit test and see if the categories corresponding to the file/path are correct.

Note Unit test are failing if executed from within the jtracker script. If they run "standalone" they produce no errors.

@mbabker I'm almost sure that you have a solution to the error:
ERROR: Serialization of 'Closure' is not allowed
that appears here: https://travis-ci.org/joomla/jissues/jobs/146807076
Note that unit test run again here with no errors...

avatar elkuku elkuku - open - 23 Jul 2016
avatar mbabker
mbabker - comment - 23 Jul 2016

You need this declaration in the test classes. The issue is that our front controllers (www/index.php and cli/tracker.php) leave variables in global scope (the DI container and path to the autoload.php file in both; the Composer autoloader instance in CLI; and the web application instance for the web requests), so when PHPUnit tries to save state and serialize the globals those variables are included in it and the container can't be serialized when it has Closures within it.

One thing that could help with that is using an IIFE in the front controllers which would cause those variables to not be globally scoped anymore.

avatar elkuku
elkuku - comment - 23 Jul 2016

Not sure if a simple "Thanks" will be enough, but if I buy you a beer instead you might become an alcoholic pretty soon ?

Actually I had the declaration right here but since I also set $runTestInSeparateProcess = true; it didn't work (only changed the error message...)

The "IIFE" thing looks scary to me at first glance (in PHP) - have to get my head around it...

well, Thank you Michael!

avatar mbabker
mbabker - comment - 23 Jul 2016

The "IIFE" thing is pretty similar to JavaScript, https://github.com/joomla/jissues/blob/master/www/media/js/jtracker.js is exactly that ?

At a glance this all looks fine.

avatar elkuku
elkuku - comment - 23 Jul 2016

@zero-24 could you take a look at this?
Especially the definitions here
and also the unit test here
and see if it's understandable and maintainable?

avatar zero-24
zero-24 - comment - 24 Jul 2016

Looks good. Maybe we can add some more Information to the dock block?

avatar elkuku
elkuku - comment - 24 Jul 2016

Makes sense - I added some words.

Any suggestions are welcome ?

avatar mbabker
mbabker - comment - 31 Aug 2016

So I sync'd this up with master and I also added matches for the PostgreSQL and SQL Server database classes to those categories (with test cases). At a glance things look good to go here for me.

avatar mbabker mbabker - change - 26 Sep 2016
Status New Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-09-26 09:53:39
Closed_By mbabker
avatar mbabker mbabker - close - 26 Sep 2016
avatar mbabker mbabker - merge - 26 Sep 2016

Add a Comment

Login with GitHub to post a comment