GPII-2515: Add code coverage reporting... #94

the-t-in-rtf · 2017-08-01T09:44:34Z

See GPII-2515 for details.

…om Grunt to npm.

gpii-bot · 2017-08-01T09:50:39Z

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

the-t-in-rtf · 2017-08-01T09:56:13Z

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 · 2017-08-01T10:03:56Z

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

… to add anchors to line numbers.

gpii-bot · 2017-08-01T11:36:09Z

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

gtirloni · 2017-08-01T12:54:04Z

ok to test

gpii-bot · 2017-08-01T13:01:39Z

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

gtirloni · 2017-08-01T13:25:05Z

@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 · 2017-08-02T11:47:47Z

CI job passed.

the-t-in-rtf · 2017-08-02T12:49:28Z

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

gtirloni · 2017-08-22T16:57:22Z

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 · 2017-08-28T12:44:02Z

@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.

This reverts commit c19067f. See GPII/linux#94 (comment)

the-t-in-rtf · 2017-10-09T14:26:48Z

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 · 2017-10-09T17:40:11Z

tests/UnitTests.sh

-pushd .
-cd ../gpii/node_modules/gsettingsBridge/tests
+cd gpii/node_modules/gsettingsBridge/tests
+# TODO: See if we can make this into a javascript-only approach
 ./runUnitTests.sh


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

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

javihernandez · 2017-10-09T17:50:02Z

tests/UnitTests.sh

 ./runUnitTests.sh
+# TODO: Produces no output when run on its own from the repo root. Investigate.


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.

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

javihernandez · 2017-10-09T17:57:51Z

tests/UnitTests.js

@@ -0,0 +1,9 @@
+// Rollup to run all javascript-only unit tests.
+"use strict";
+require("../gpii/node_modules/alsa/test/alsaSettingsHandlerTests.js");


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

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

…s taken care of using npm scripts.

gpii-bot · 2017-10-12T12:01:59Z

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

the-t-in-rtf · 2017-10-12T12:05:03Z

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 · 2017-10-12T12:09:53Z

@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 · 2017-10-12T12:11:54Z

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

the-t-in-rtf · 2017-12-04T13:02:23Z

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

javihernandez · 2017-12-20T17:34:48Z

ok to test

gpii-bot · 2017-12-20T17:42:00Z

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

the-t-in-rtf · 2017-12-21T09:09:47Z

@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 · 2017-12-21T14:15:33Z

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 · 2017-12-21T14:46:34Z

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 · 2017-12-21T17:01:14Z

@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 · 2018-02-14T01:45:43Z

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 · 2018-02-15T09:40:07Z

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 · 2018-02-15T09:46:56Z

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

the-t-in-rtf · 2018-03-07T14:07:32Z

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 · 2018-04-02T14:47:13Z

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 · 2018-04-03T08:46:43Z

@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 · 2018-05-23T10:44:56Z

@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

GPII-2515: Added code coverage reporting and migrated test scripts fr…

aa022b6

…om Grunt to npm.

GPII-2515: Updated universal to pick up new xrandr tests and istanbul…

7a00d8a

… to add anchors to line numbers.

the-t-in-rtf added 2 commits August 2, 2017 13:37

GPII-2515: Fixed typo in test:vagrantAcceptance script definition.

ef44933

Merge remote-tracking branch 'upstream/master' into GPII-2515

1736769

gtirloni mentioned this pull request Aug 22, 2017

GPII-1939, GPII-2395 - Device Reporter reports screen resolutions #95

Open

gtirloni added a commit to GPII/ci-service that referenced this pull request Aug 28, 2017

Revert "GPII-2515 - Switch to new npm run commands"

6fd1fa4

This reverts commit c19067f. See GPII/linux#94 (comment)

javihernandez requested changes Oct 9, 2017

View reviewed changes

the-t-in-rtf added 5 commits October 12, 2017 13:50

GPII-2515: Removed unneeded shell test, now that the required setup i…

3435914

…s taken care of using npm scripts.

GPII-2515: Converted gsettings tests to use jqUnit.

868ebe3

GPII-2515: Added additional ALSA tests per PR feedback.

17229e6

GPII-2515: Fixed calls to ok method to use assertTrue.

db4b892

GPII-2515: Added node gsettings tests to unit test rollup.

fe15eae

the-t-in-rtf added 3 commits February 15, 2018 10:15

Merge remote-tracking branch 'upstream/master' into GPII-2515

1f5327a

GPII-2515: Updated to latest universal master.

f1e851b

GPII-2515: Committed latest package lock file.

21b322b

javihernandez merged commit 21b322b into GPII:master May 23, 2018

gtirloni mentioned this pull request May 23, 2018

GPII-2515 - Adjust build commands GPII/ci-service#43

Merged

		./runUnitTests.sh
		# TODO: Produces no output when run on its own from the repo root. Investigate.

GPII-2515: Add code coverage reporting... #94

GPII-2515: Add code coverage reporting... #94

Conversation

the-t-in-rtf commented Aug 1, 2017

gpii-bot commented Aug 1, 2017

the-t-in-rtf commented Aug 1, 2017

the-t-in-rtf commented Aug 1, 2017

gpii-bot commented Aug 1, 2017

gtirloni commented Aug 1, 2017

gpii-bot commented Aug 1, 2017

gtirloni commented Aug 1, 2017

gpii-bot commented Aug 2, 2017

the-t-in-rtf commented Aug 2, 2017

gtirloni commented Aug 22, 2017

the-t-in-rtf commented Aug 28, 2017

the-t-in-rtf commented Oct 9, 2017

javihernandez left a comment

Choose a reason for hiding this comment

javihernandez Oct 9, 2017

Choose a reason for hiding this comment

the-t-in-rtf Oct 12, 2017

Choose a reason for hiding this comment

the-t-in-rtf Oct 12, 2017

Choose a reason for hiding this comment

javihernandez Oct 9, 2017

Choose a reason for hiding this comment

the-t-in-rtf Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

the-t-in-rtf Oct 12, 2017

Choose a reason for hiding this comment

javihernandez Oct 9, 2017

Choose a reason for hiding this comment

the-t-in-rtf Oct 12, 2017

Choose a reason for hiding this comment

gpii-bot commented Oct 12, 2017

the-t-in-rtf commented Oct 12, 2017 • edited Loading

the-t-in-rtf commented Oct 12, 2017

gpii-bot commented Oct 12, 2017

the-t-in-rtf commented Dec 4, 2017

javihernandez commented Dec 20, 2017

gpii-bot commented Dec 20, 2017

the-t-in-rtf commented Dec 21, 2017

javihernandez commented Dec 21, 2017

gtirloni commented Dec 21, 2017

avtar commented Dec 21, 2017

javihernandez commented Feb 14, 2018

the-t-in-rtf commented Feb 15, 2018

gpii-bot commented Feb 15, 2018

the-t-in-rtf commented Mar 7, 2018

gtirloni commented Apr 2, 2018

the-t-in-rtf commented Apr 3, 2018

javihernandez commented May 23, 2018

the-t-in-rtf Oct 12, 2017 •

edited

Loading

the-t-in-rtf commented Oct 12, 2017 •

edited

Loading