GPII-2515: Add code coverage reporting...

[the-t-in-rtf]

See GPII-2515 for details.

[gpii-bot]

CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details.

[the-t-in-rtf]

As with the similar universal PR, the tests will not pass here until the CI job is updated. As with that PR, I propose that we review, then update the CI job and confirm passing as a final step before merging.

[the-t-in-rtf]

For those who are curious, here is a copy of the code coverage report I generated earlier today.

[gpii-bot]

CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details.

[gtirloni]

ok to test

[gpii-bot]

CI job failed. Please visit http://lists.gpii.net/pipermail/ci/ for more details.

[gtirloni]

@the-t-in-rtf looks like there's a small typo in package.json (missing run in npm test:acceptance).

Could you also update the Fedora box to version 26 in the Vagrant file (or merge 'master')?

[gpii-bot]

CI job passed.

[the-t-in-rtf]

Thanks, @gtirloni, good catch on the typo. I merged with master as well and confirmed working with the Fedora 26 box.

[gtirloni]

This was mentioned in the architecture meeting on 8/9 and it's blocking other PRs (e.g. #95).

I can revert the CI configuration if the review for this PR will take more time.

[the-t-in-rtf]

@gtirloni, although it's nice to know this PR will eventually pass once the CI config is updated, it makes more sense to update the CI config after it's been reviewed, but before it's merged. That's what I was trying to propose earlier. I'd suggest reverting the CI config, we can discuss the timeline (and hopefully find someone to actually review this) in the meeting.

[the-t-in-rtf]

Hi, @javihernandez. Just seeing if you have any bandwidth to look at this, it's just adding code coverage reporting to the linux repo.

[javihernandez]

Hey @the-t-in-rtf, sorry for the long delay in reviewing this. I've been taking a look at it and, in general, it looks very good. Can you please update it according to the review in order to get rid of UnitTests.sh?
BTW, thanks for such improvement 👍

[javihernandez]

I guess that we will get rid of this shell script (UnitTests.sh) but it won't run after your refactoring of runUnitTests.sh.

[the-t-in-rtf]

As soon as I confirm the node_gsettings tests will run, I'll get rid of it.

[the-t-in-rtf]

Done.

[javihernandez]

This won't produce any output unless there's an error as nodegsettings_tests.js uses the assert module. I'd say that these tests must be updated to use jqUnit so we can get the output even if they're not failing. In any case, you can just add it into UnitTests.js so it runs like the others and we can just get rid of this script. The side-effect of this is that we won't get the results of the rest of the tests when this one (nodegsettings_tests.js) fails until we refactor the tests to use jqUnit.

[the-t-in-rtf]

I'll rewrite the tests to use node-jqUnit, that seems too easy to not just take care of while I'm here.

[the-t-in-rtf]

Done.

[javihernandez]

https://github.com/GPII/linux/blob/master/gpii/node_modules/alsa/nodealsa/nodealsa_tests.js must be included too.

[the-t-in-rtf]

Good catch on the missing test. I searched to make sure I had the rest.

[gpii-bot]

CI job failed: https://ci.gpii.net/job/linux-tests/114/

[the-t-in-rtf]

Thanks for the feedback, @javihernandez. I have updated the PR as suggested. I went ahead and converted the gsettings tests to use jqUnit. Following these changes, here is an example of the current coverage output. This should be ready for further review.

[the-t-in-rtf]

@javihernandez, just to confirm, the idea is, once you're happy, we ask @gtirloni to update the CI config, confirm the builds pass, and merge.

[gpii-bot]

CI job failed: https://ci.gpii.net/job/linux-tests/115/

[the-t-in-rtf]

Hi, @javihernandez, as far as I can see, I addressed your feedback a while ago. Can you please take a look again?

[javihernandez]

ok to test

[gpii-bot]

CI job failed: https://ci.gpii.net/job/linux-tests/125/

[the-t-in-rtf]

@javihernandez, this pull replaces the previous Grunt infrastructure, so the CI jobs will not work until the configuration is updated. As with similar pulls against other repos, we need to verify this using vagrant up && npm run test:vagrant for now, and once we're satisfied, discuss switching the CI builds for this repo to use the new commands, then merge the pull once the updated builds run successfully.

That's assuming we're still talking about the existing Jenkins jobs. If this is scheduled to be updated to use buildkite, we could test a new configuration as part of the pull. @avtar?

[javihernandez]

Hi @the-t-in-rtf, right, I remembered that right after checking on the logs why this was failing on CI. 🤦‍♂️

I've just created a fresh vagrant box and I see that it's using node8/npm5, so I guess that we need to revert this situation until we find the right moment to do the switch (eventually, we can keep node 8 but we need to use an older version of npm). @gtirloni @amatas?

[gtirloni]

For Linux, the desired npm version can be selected in the provisioning/vars.yml file (see https://github.com/GPII/linux/blob/master/provisioning/vars.yml#L14-L15).

[avtar]

@the-t-in-rtf At the moment there aren't any plans for switching GPII projects to using Buildkite. That work was done for Infusion since PRs weren't being tested.

[javihernandez]

Hi @the-t-in-rtf,

I went through this one again and I had difficulties to run the acceptance tests after merging it with current linux/master. There is an underlying problem with the version of universal that you are introducing in this pr. Luckily, I updated the universal reference to point to current universal/master and it solved the issue. Can you update this pr:

• to be in sync with linux/master
• to update the universal reference with the last one

Thanks again for your work on this 👍

[the-t-in-rtf]

Hi, @javihernandez. I pulled in the recent changes to linux (only the Vagrant config, looks like) and updated to the latest commit of universal. The tests and code coverage report now run as expected, here's an example of the output:

https://the-t-in-rtf.github.io/coverage-reports/linux/20180215/

[gpii-bot]

CI job failed: https://ci.gpii.net/job/linux-tests/137/

[the-t-in-rtf]

Just to confirm, this pull also migrates away from using grunt tasks, in favour of npm scripts. The CI builds will not work until we agree this is far enough along to update the config to simply run "npm test".

[gtirloni]

Tested this PR manually with the newest version of the Fedora 27 box (20180330).

Unit:

$ npm run test:vagrantUnit
...
14:45:40.835:  jq: ***************
14:45:40.836:  jq: All tests concluded: 114/114 total passed in 48813ms - PASS
14:45:40.836:  jq: ******

Acceptance:

$ npm run test:vagrantAcceptance

14:46:46.153: jq: ***************
14:46:46.153: jq: All tests concluded: 122/122 total passed in 20321ms - PASS
14:46:46.153: jq: ***************

[the-t-in-rtf]

@javihernandez, as far as I can see all we need to do is agree to update the CI config and encourage everyone with an open pull to bring in these changes. I will bring it up at tomorrow's meeting.

[javihernandez]

@the-t-in-rtf thanks for your work on this!
I guess that now we must:

• Ask devops to update our CI
• Ask developers to update their pull requests against this repo