Create an example that includes tests and how to run them. #122 #130

hmelenok · 2016-10-30T13:53:03Z

Please check #122

* icons with mocha * initial web-extension * dependencies and npm configs

wbamberg · 2016-11-03T16:29:34Z

@andymckay , do you know who might be qualified to review this? There's a lot of stuff in here I don't know, so while I could probably figure out whether it's working properly, I probably won't be able to tell whether it's good practice.

andymckay · 2016-11-03T18:31:48Z

@kumar303 might be able to help.

andymckay · 2016-11-03T18:32:55Z

mocha-client-tests/addon/manifest.json

+  },
+  "applications": {
+    "gecko": {
+      "id": "production@gemshelf.com",


We've been trying to remove the id out of example extensions, they can be through temporary install locally or pushed to AMO for signing. If there's no id, they won't all conflict with each other.

Oh sorry will remove today

Copy paste mistake

Please check it

kumar303

Thanks for getting this started! I requested some changes and also asked some questions to help me understand it better.

kumar303 · 2016-11-04T20:45:41Z

mocha-client-tests/addon/package.json

+    "mocha": "^3.1.2",
+    "should": "^11.1.1"
+  }
+}


Why is there a separate package.json inside the addon dir? I was able to run the add-on and also run the tests with npm run test without installing any packages within the addon dir. If it's not necessary we shouldn't include it.

There two type of tests inside of addon i had add mocha client tests

Outside of addon folder there phantomjs tests

kumar303 · 2016-11-04T20:53:13Z

mocha-client-tests/addon/scripts/popup.js

@@ -0,0 +1,16 @@
+/**
+ * Created by hmelenok on 10/30/16.


It looks like your editor (or something) added these automatically. Can you remove them all? They don't appear to be relevant to the example so they might be distracting.

Done removed

kumar303 · 2016-11-04T21:01:42Z

mocha-client-tests/tests/lib/background.test.js

+    describe('ping', function() {
+        it('should return pong in response', function() {
+            Background.ping(false, false, function(response) {
+                response.should.equal('pong');


chai's prototype hack might be confusing to the casual JavaScript reader. Could you update this to use expect() (or something) instead? It would look like expect(response).to.equal('pong')

Thanks, changed to expect 👍

I don't see the change to make this expect(). Do you have any unpushed commits maybe?

kumar303 · 2016-11-04T21:21:14Z

mocha-client-tests/addon/tests/lib/background-messaging.test.js

+ */
+describe('Background', function() {
+    describe('ping', function() {
+        it('should return pong in response', function(done) {


how do I run this test? It isn't executed with npm run test from the root directory. Is there a different way to run it?

npm run test - runs phantomjs tests, there UI ob mocha which included as example if you install all dependencies, please read README.md before

kumar303 · 2016-11-04T21:23:38Z

mocha-client-tests/README.md

+##PhantomJs tests
+`npm run test` will run simple test  `./addon/background.js`
+## Mocha browser test
+Already inside of addon 


What do you mean by this? I couldn't figure out how to run the tests in addon/tests/lib. How do I run them and see results?

cd mocha-client-tests/addon && npm i - it will install all node modules which required for running tests in addon window

Then just run web-ext run -s ./addon with cli

hmelenok · 2016-11-05T17:45:57Z

Please check last changes:

removed comments from js files
removed jquery and empty contentscript
changed should to expect

kumar303

Thanks for fixing up the mocha test result view. I understand the two test suites better now. I ran into a few problems that need fixing.

kumar303 · 2016-11-07T17:00:55Z

mocha-client-tests/addon/tests/lib/test.array.js

+describe('Array', function() {
+    describe('#indexOf()', function() {
+        it('should return -1 when the value is not present', function() {
+            [1,2,3].indexOf(5).should.equal(-1);


This fails with should is undefined; it needs to use expect() instead, right?

kumar303 · 2016-11-07T17:02:32Z

mocha-client-tests/README.md

+And inside of addon please run `npm i`
+
+##Run with web-ext cli
+If you have it already run `npm run web-ext` if not `npm i web-ext -g` and then first command (will work with FF dev edition), if you have error with web-ext cli please add path for FF binary file with `--firefox-binary /path/to/firefox-bin`


npm run web-ext doesn't work because package.json wants it to be npm run web-ext-run. I think you should rename the script in package.json to web-ext.

kumar303 · 2016-11-07T17:03:23Z

mocha-client-tests/README.md

+And inside of addon please run `npm i`
+
+##Run with web-ext cli
+If you have it already run `npm run web-ext` if not `npm i web-ext -g` and then first command (will work with FF dev edition), if you have error with web-ext cli please add path for FF binary file with `--firefox-binary /path/to/firefox-bin`


npm install -g web-ext is not necessary because you've already put it in the devDependencies. When you do npm run it figures out the path to web-ext automatically.

Sorry you right

kumar303 · 2016-11-07T17:04:04Z

mocha-client-tests/README.md

+##Install dependency
+`npm install` and `bower install`  
+
+And inside of addon please run `npm i`


I think it would be more helpful to spell out npm install in all cases since not all readers will know about the i alias.

kumar303 · 2016-11-07T17:05:08Z

mocha-client-tests/README.md

+And inside of addon please run `npm i`
+
+##Run with web-ext cli
+If you have it already run `npm run web-ext` if not `npm i web-ext -g` and then first command (will work with FF dev edition), if you have error with web-ext cli please add path for FF binary file with `--firefox-binary /path/to/firefox-bin`


These instructions need a final sentence explaining that you need to click the mocha icon from the browser bar to run the tests

kumar303 · 2016-11-07T17:27:30Z

mocha-client-tests/addon/tests/lib/background-messaging.test.js

+    describe('ping', function() {
+        it('should return pong in response', function(done) {
+            chrome.runtime.sendMessage({action: 'ping'},function(response) {
+                expect(response).to.equal('pong');


when this assertion fails, it only gets logged to the browser console. Can you make it show up in the mocha test output instead? For example, if you change expect(response).to.equal('pong') to expect(response).to.equal('nope') then I see this confusing timeout instead of the assertion error:

mochajs/mocha#1128 - looks like I'm not sure why it wait until fail

ok, I see what the issue is now. The expect() call throws an error but that is not passed to done(error) so the mocha reporter can't show it. The way to fix this is to rewrite the code using promises:

it('should return pong in response', function() { // Return a promise for Mocha using the Firefox browser API instead of chrome. return browser.runtime.sendMessage({action: 'ping'}) .then(function(response) { expect(response).to.equal('pong'); }); });

I think it would be good to convert this code into promises since the developer can always use a polyfill to make it compatible with Chrome.

Thanks for advice it works!

Added in last commit

kumar303 · 2016-11-07T17:29:28Z

mocha-client-tests/addon/manifest.json

+    "default_title": "Mocha Test",
+    "default_popup": "popup.html"
+  },
+  "applications": {


You can remove this section entirely. It's not needed for this example. It could be harmful if someone copy/pasted it because it makes the extension only compatible with Firefox, not Chrome.

kumar303 · 2016-11-07T17:31:20Z

mocha-client-tests/addon/manifest.json

+      "background.js"
+    ]
+  },
+  "permissions": [


This looks like it has a bunch of permissions not needed for this example. Can you strip it down to only the relevant permissions? I think only notifications is needed, right?

kumar303 · 2016-11-07T17:32:55Z

mocha-client-tests/addon/tests/mocha-run.js

@@ -0,0 +1,3 @@
+mocha.checkLeaks();
+mocha.globals(['jQuery','AppView', 'ExtensionOptions', 'ExtensionView', 'WebView']);


where are these globals used? These don't look like they are relevant to the example

There a list of default globals in extension which from Mocha perspective looks like global leaks

ok, can you add a comment above this line of code explaining that?

kumar303 · 2016-11-07T17:33:55Z

mocha-client-tests/package.json

+  "license": "MPL-2.0",
+  "devDependencies": {
+    "chai": "^3.5.0",
+    "chrome-mock": "0.0.9",


where is this used?

chrome-mock used for phantomJS as it not have WebExtensions (or Chrome Extensions) API inside of engine, so i'm using mock of this API's.
When you include to test runner original file(at my point background.js) it throws errors about chrome is not defined

…reenshot, removed bower from

kumar303 · 2016-11-08T15:39:49Z

I see that you pushed some more changes (thanks!). Just let me know when it's ready for another review.

hmelenok · 2016-11-11T10:20:49Z

Please check it again)

kumar303

Sorry for the delay in getting back to you. This is really close.

kumar303 · 2016-11-18T23:15:42Z

mocha-client-tests/README.md

+
+When addon will start click on mocha icon in your browser bar to run client tests: 
+
+![addon screenshot](screenshots/addon-button.png "Mocha test addon")


good idea to add a screenshot

kumar303 · 2016-11-18T23:23:14Z

mocha-client-tests/tests/lib/background.test.js

+    describe('ping', function() {
+        it('should return pong in response', function() {
+            Background.ping(false, false, function(response) {
+                response.should.equal('pong');


I don't see the change to make this expect(). Do you have any unpushed commits maybe?

kumar303 · 2016-11-18T23:23:43Z

mocha-client-tests/addon/scripts/browser-polyfill.js

@@ -0,0 +1,849 @@
+/* webextension-polyfill - v0.1.0 - Wed Nov 09 2016 16:37:59 */


this is the built dist file, right? I don't know what to look for.

That what i get after build in webextension-polyfill

kumar303 · 2016-11-30T23:09:12Z

Thanks for all the fixups!

hmelenok added 8 commits October 30, 2016 14:57

added initial tests:

50a52c7

* icons with mocha * initial web-extension * dependencies and npm configs

.idea to gitignore

4f22369

ping-pong tests

b653a15

ping-pong tests

2d1e054

renamed folder

678cc14

phantomjs tests

3aae097

README

99ceaad

README (forgot about addon folder)

2340a82

andymckay reviewed Nov 3, 2016

View reviewed changes

removed id

7770bad

kumar303 suggested changes Nov 4, 2016

View reviewed changes

kumar303 self-assigned this Nov 4, 2016

hmelenok added 2 commits November 5, 2016 19:27

removed autocomments and not used contentscript

f549d15

removed jquery to plain js

3c7c338

removed should to expect

3b59bd5

kumar303 suggested changes Nov 7, 2016

View reviewed changes

hmelenok added 5 commits November 8, 2016 10:58

removed should to expect, removed permissions from manifest, added sc…

b169522

…reenshot, removed bower from

changed readme => npm test

25c614a

change readme removed Mocha section

507cfcb

changed path in readme

7a80856

changed path in readme

1acbcf6

Added browser webextension-polyfill + comment on global leaks

2fd35d5

kumar303 mentioned this pull request Nov 9, 2016

Experiment with testing web extension examples mozilla/web-ext#162

Open

kumar303 suggested changes Nov 18, 2016

View reviewed changes

changed polifill to minified version + should => expect

17d4075

kumar303 merged commit 395786e into mdn:master Nov 30, 2016

kumar303 mentioned this pull request Nov 30, 2016

Create an example that includes tests and how tor un them. #122

Closed

		@@ -0,0 +1,3 @@
		mocha.checkLeaks();
		mocha.globals(['jQuery','AppView', 'ExtensionOptions', 'ExtensionView', 'WebView']);


		When addon will start click on mocha icon in your browser bar to run client tests:

		![addon screenshot](screenshots/addon-button.png "Mocha test addon")

		@@ -0,0 +1,849 @@
		/* webextension-polyfill - v0.1.0 - Wed Nov 09 2016 16:37:59 */

Create an example that includes tests and how to run them. #122 #130

Create an example that includes tests and how to run them. #122 #130

Conversation

hmelenok commented Oct 30, 2016

wbamberg commented Nov 3, 2016

andymckay commented Nov 3, 2016

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

kumar303 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmelenok commented Nov 5, 2016 • edited

kumar303 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

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

hmelenok Nov 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 commented Nov 8, 2016

hmelenok commented Nov 11, 2016

kumar303 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

kumar303 commented Nov 30, 2016

hmelenok commented Nov 5, 2016 •

edited

hmelenok Nov 8, 2016 •

edited