Updated .travis.yml to support Moodlerooms plugin #119

mikemcgowan · 2017-06-14T12:50:44Z

I have now updated the .travis.yml file to support the Moodlerooms Travis plugin.

This wasn't straightforward because this plugin executes its own PHPUnits outside of the Moodle PHPUnit test environment. (This should probably be changed so that this plugin's PHPUnits are executable within the wider Moodle PHPUnit test environment. Then, the moodle-plugin-ci phpunit item can be commented-in in .travis.yml and the $ORIG hack removed - see below.)

To avoid the Moodlerooms Travis plugin from linting the Composer packages required by this plugin, I had to cache the original working directory of the plugin (as $ORIG). As the very last item in the Travis build, after all the Moodlerooms stuff is finshed, this plugin's Composer packages are installed and then its PHPUnits ran (independently from the Moodle PHPUnit test environment).

The build reaches the end successfully but of course, there are many errors because the code doesn't currently lint to the Moodle coding style.

However, the second two of these three items pass:

    - moodle-plugin-ci codechecker
    - moodle-plugin-ci validate
    - moodle-plugin-ci savepoints

The plugin's own PHPUnits also pass, as previously (of which there are currently 71 tests, 2304 assertions).

ryasmi · 2017-06-14T13:07:41Z

Nice @mikemcgowan

mikemcgowan · 2017-06-28T09:55:44Z

@ryansmith94 any word on getting this merged?

ryasmi

Happy for this to be merged once all of the code checker issues have been resolved and my code comments have been addressed 👍

ryasmi · 2017-06-28T16:02:01Z

.travis.yml

+    - moodle-plugin-ci install
+
+script:
+#   - moodle-plugin-ci phplint


What's the reason for commenting out each of these lines?

Which? Specifically moodle-plugin-ci phpcpd and moodle-plugin-ci phpmd? I suppose, in the first instance, I wanted to get a smaller subset of the available Moodlerooms stuff passing - in particular moodle-plugin-ci codechecker so the code lints to Moodle's own standard.

At a later date, once the Travis build is passing against the Moodlerooms plugins that are currently commented-in, some of the additional Moodlerooms plugins could also be commented in, and any issues then fixed. I think the phpcpd (copy/paste detector) and phpmd (mess detector) are probably very useful checks to include, in the long run, but as there's going to be a tonne of codechecker failures to fix first, I suppose it's arguably better to do each plugin at a time?

I see, this makes a lot of sense. Yeah we'll enable one bit at a time. I agree those bits should be useful.

ryasmi · 2017-06-28T16:04:58Z

.travis.yml

+#   - moodle-plugin-ci phplint
+#   - moodle-plugin-ci phpcpd
+#   - moodle-plugin-ci phpmd
+    - moodle-plugin-ci codechecker


Is there a way to run these things locally?

As far as moodle-plugin-ci codechecker goes, it's possible to run phpcs locally against the Moodle coding style by following these instructions.

It's apparently possible to run a Travis build locally so that's possibly a way in which to run the other moodle-plugin-ci items.

Okay. probably wouldn't want to run Travis builds locally though, seems a bit overkill. As long as we can run the code checker locally I'm happy.

ryasmi · 2017-06-28T16:06:29Z

.travis.yml

+env:
+    global:
+        - MOODLE_BRANCH=MOODLE_32_STABLE
+    matrix:


This is probably not needed. I'm fairly certain this exists so that the tests are ran against those DBs but since we test against memory to improve build times this isn't required.

At the moment, the existing tests don't run in the context of Moodle's PHPUnit test framework and PHPUnit test database. I appreciate that the existing PHPUnits have been written in such a way that a test database isn't currently required, however, it's a Moodle plugin and its tests should really be executable as part of the Moodle PHPUnit test framework.

It's certainly possible that, in the future, test coverage of functionality that touches the database may benefit from being written (e.g. test coverage of the functionality that processes the queue of unsent statements in the logstore_xapi_log table). In which case, the test database would end up being necessary anyway.

Finally, my guess (I haven't checked this) is that the Moodlerooms stuff relies on Moodle actually being installed, so my guess is that the above probably does have to be there (I've taken the .travis.yml file from here so it's very close to the Moodlerooms distribution file).

Ok in that case, let's comment one out for now, otherwise we're running 6 builds when we could be running 3.

Okay. Done.

mikemcgowan · 2017-06-28T17:21:05Z

I've pushed another commit which addresses the Moodle coding style everywhere but the lib/ directory.

I'll do the lib/ directory hopefully within the next couple of days.

mikemcgowan · 2017-06-28T17:24:17Z

By the way, as part of this PR, Code Climate should be turned off, but I don't think I can do that myself?

mikemcgowan · 2017-06-29T10:23:25Z

I've now done lib/emitter, just lib/expander and lib/translator to go.

It's fairly time-consuming, though!

ryasmi · 2017-06-29T11:23:47Z

@mikemcgowan awesome work!!! I imagine this is really time consuming, really appreciate you taking the time to get this done 👍

I will disable code climate.

…ot tests passing under Moodle's PHPUnit test framework

mikemcgowan · 2017-06-29T21:49:32Z

@ryansmith94 I'm pretty sure I've done everything now:

I've squashed the PR down to just two commits. The first is just .travis.yml (as I originally pushed when I first opened the PR), the second is everything else that I've done over the last couple of days.
All the plugin's code now lints to Moodle's coding style (except the vendor/ directory, obviously!).
All the tests that are currently in master also pass here (71 tests, 2304 assertions).
I've got the tests working with the Moodle PHPUnit test framework (they still run quite quickly as they don't touch the database). And I've documented this in docs/developers.md.
I've moved all the tests into the tests/ directory, because that's where Moodle expects them.

Hopefully, all's good for getting this merged ...

ryasmi

Wow @mikemcgowan, you're a legend, this is just awesome and I'm massively impressed! Just got one comment on the .gitignore, but I'm happy to approve this, just want to know where those ignored files are coming from.

ryasmi · 2017-06-30T10:29:54Z

docs/developers.md

@@ -26,10 +26,23 @@ If you've read the [plugin design](design.md) you should understand what each of
 - [Moodle to xAPI Translator](https://github.com/LearningLocker/Moodle-xAPI-Translator/blob/master/docs/readme.md#adding-events)
 - [xAPI Recipe Emitter](https://github.com/LearningLocker/xAPI-Recipe-Emitter/blob/master/docs/readme.md#adding-events)

-##PHPUnit Test Filter
-All PHPUnit tests should pass, but if you'd like to only run the tests for specific events, add the 'filter' option.
+## PHPUnit


Good thinking 👍

ryasmi · 2017-06-30T10:30:27Z

.gitignore

@@ -5,3 +5,4 @@ vendor
 classes/log/error_log.txt
 composer.phar
 .idea/
+lib/*/docs/examples/*_test.json


Why is this ignored now?

Actually what is that?

These files get generated by some of the PHPUnits. Now that the .php files with tests have been changed to the Moodle standard test filename (i.e. *_test.php) the generated test files have different filenames.

The generation of these JSON files by the PHPUnits was already happening in master, and those automatically generated files have already been committed, e.g. see 1139e3f

I wasn't brave enough to take the decision to remove the existing ones! I thought it was safer/cleaner to .gitignore the new ones with the *_test.json extension.

+$ git diff master | grep -Pi '\$exampleFile' - $exampleFile = __DIR__.'/../docs/examples/'.$eventName.'.json'; - file_put_contents($exampleFile, json_encode($output, JSON_PRETTY_PRINT)); - $exampleFile = __DIR__.'/../docs/examples/'.$eventName.'.json'; - file_put_contents($exampleFile, json_encode($output, JSON_PRETTY_PRINT)); - $exampleFile = __DIR__.'/../docs/examples/'.$eventName.'.json'; - file_put_contents($exampleFile, json_encode($output, JSON_PRETTY_PRINT)); + $examplefile = __DIR__ . '/../../../lib/emitter/docs/examples/' . $eventname . '.json'; + file_put_contents($examplefile, json_encode($output, JSON_PRETTY_PRINT)); + $examplefile = __DIR__ . '/../../../lib/expander/docs/examples/' . $eventname . '.json'; + file_put_contents($examplefile, json_encode($output, JSON_PRETTY_PRINT)); + $examplefile = __DIR__ . '/../../../lib/translator/docs/examples/' . $eventname . '.json'; + file_put_contents($examplefile, json_encode($output, JSON_PRETTY_PRINT));

Ah. You've highlighted a small issue, actually, with the generation of those JSON files. They're not actually supposed to have the _test extension:

protected function create_example_file($output) { $classarray = explode('\\', get_class($this)); $eventname = str_replace('Test', '', array_pop($classarray)); $examplefile = __DIR__ . '/../../../lib/emitter/docs/examples/' . $eventname . '.json'; file_put_contents($examplefile, json_encode($output, JSON_PRETTY_PRINT)); }

I should have changed the str_replace line to take into consideration the change of filename.

I'll sort that out now.

I've pushed another commit to address this issue.

The change to .gitignore is no longer necessary so I've backed it out.

The automatic generation of the file name of the example event JSON files by the PHPUnit tests has been corrected (changed from replacing Test to replacing _test).

Example event JSON files themselves (in lib/*/docs/examples/) have all been renamed

Nice! This is awesome! Thanks @mikemcgowan 👍

Stale

ryasmi · 2017-06-30T12:47:15Z

Have a quick check of this if you like @davidpesce, I think it's ready to merge.

davidpesce · 2017-06-30T15:51:02Z

Wow, this looks like quite a bit of work. Thanks, @mikemcgowan !

After a quick look, this looks OK to me. I haven't done a full test, but if you are good with it, merge it.

Updated .travis.yml to support Moodlerooms plugin

49a9f21

ryasmi previously requested changes Jun 28, 2017

View reviewed changes

Applied Moodle coding style, moved all tests into tests/ directory, g…

67f80fb

…ot tests passing under Moodle's PHPUnit test framework

ryasmi reviewed Jun 30, 2017

View reviewed changes

Corrected generated file name of example event JSON files

1dbf2c1

ryasmi approved these changes Jun 30, 2017

View reviewed changes

ryasmi mentioned this pull request Jun 30, 2017

Statement Templates #124

Closed

ryasmi merged commit 0f00cae into xAPI-vle:master Jul 3, 2017

mikemcgowan deleted the travis branch July 3, 2017 08:34

This was referenced Jul 3, 2017

Code does not follow the Moodle coding style #117

Closed

It's no longer necessary to instantiate \MXTranslator\Events\Event and invoke its 'read' method just to get at the 'context' ext key #112

Closed

ryasmi mentioned this pull request Oct 10, 2017

Error when validating logstore_xapi #128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated .travis.yml to support Moodlerooms plugin #119

Updated .travis.yml to support Moodlerooms plugin #119

mikemcgowan commented Jun 14, 2017

ryasmi commented Jun 14, 2017

mikemcgowan commented Jun 28, 2017

ryasmi left a comment

ryasmi Jun 28, 2017

mikemcgowan Jun 28, 2017

ryasmi Jun 29, 2017

ryasmi Jun 28, 2017

mikemcgowan Jun 28, 2017

ryasmi Jun 29, 2017

ryasmi Jun 28, 2017

mikemcgowan Jun 28, 2017

ryasmi Jun 29, 2017

mikemcgowan Jun 29, 2017

mikemcgowan commented Jun 28, 2017

mikemcgowan commented Jun 28, 2017

mikemcgowan commented Jun 29, 2017

ryasmi commented Jun 29, 2017

mikemcgowan commented Jun 29, 2017

ryasmi left a comment

ryasmi Jun 30, 2017

ryasmi Jun 30, 2017

ryasmi Jun 30, 2017

mikemcgowan Jun 30, 2017

mikemcgowan Jun 30, 2017

mikemcgowan Jun 30, 2017

ryasmi Jun 30, 2017

ryasmi commented Jun 30, 2017

davidpesce commented Jun 30, 2017

Updated .travis.yml to support Moodlerooms plugin #119

Updated .travis.yml to support Moodlerooms plugin #119

Conversation

mikemcgowan commented Jun 14, 2017

ryasmi commented Jun 14, 2017

mikemcgowan commented Jun 28, 2017

ryasmi 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

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

mikemcgowan commented Jun 28, 2017

mikemcgowan commented Jun 28, 2017

mikemcgowan commented Jun 29, 2017

ryasmi commented Jun 29, 2017

mikemcgowan commented Jun 29, 2017

ryasmi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryasmi commented Jun 30, 2017

davidpesce commented Jun 30, 2017