Fixes #90. Add Intern unit tests for isReportableUrl. #116

laghee · 2018-05-17T20:44:47Z

r? @miketaylr

Lots of messy committing! (And I might have missed something in my delirium, just let me know.)

Edit: OK, more messy commit snarls, but I think I got all the leftover comments & the very, very sad comma.

miketaylr · 2018-05-17T20:53:25Z

Awesome!

Can you check out the error message from Travis?

https://travis-ci.org/webcompat/webcompat-reporter-extensions/builds/380409861#L465

(I should be able to review this tomorrow morning)

package.json

    "web-ext": "^2.4.0",
    "webpack": "^4.1.1",
    "webpack-cli": "^2.0.10"
  },
-  "license": "MPL-2.0"
+  "license": "MPL-2.0",


tests/lib/helpers.js

@@ -2,11 +2,14 @@
 * http://creativecommons.org/publicdomain/zero/1.0/ */

 const webpack = require("webpack");
+// import webpack from "webpack";


tests/lib/helpers.js


 /*
  Returns a promise that resolves once webpack has compiled all the addon
  configs have been built.
 */
+// export default function


tests/test-manifest.js

 const { registerSuite } = intern.getInterface("object");
 const { assert } = intern.getPlugin("chai");

-const helpers = require("./lib/helpers");
-const shell = require("shelljs");
+// const helpers = require("./lib/helpers");


miketaylr

This is awesome! Just one tiny nit, can you add a newline to .babelrc?

Then we just need to clean up the history, but we can also just punt and squash everything.

.babelrc

@@ -0,0 +1,3 @@
+{
+  "plugins": ["transform-es2015-modules-commonjs"]
+}


intern.json

+  "node": {
+    "plugins":
+      "node_modules/babel-register/lib/node.js"


tests/test-checkurl-import.js

+    "calling isReportableURL with invalid protocols will not return true"() {
+      // Verify that calling isReportableURL with invalid protocol does not return true.
+      let guacUrl = "guacamole://yumavocados.com";
+      let ftpUrl = "ftp://downloadfreetacos.com";


miketaylr · 2018-05-18T15:37:43Z

One tiny tip for rebasing against master so you don't end up with merge commits.

Since you're doing this from your fork, you need to make sure you have an upstream remote:

git remote -v will tell you that, and if you don't (but I assume you do?), https://help.github.com/articles/configuring-a-remote-for-a-fork/ has some help on that.

So the first step is to sync up remotes:

git remote update

This won't do anything destructive, it'll just make sure it knows about the latest changes on the server.

Then you want to do the rebase, which essentially means to put your local patches on top of any new stuff on master.

git pull --rebase upstream master

This is saying, pull in the latest stuff from the master branch on upstream remote, but as a rebase.

Assuming that works, you'll see something like "replaying your commits on top of blah".

But at this point, your local branch will be different from whatever you've already pushed to GitHub, so if you try to git push you'll get rejected. That's OK. It just means you have to force push:

git push -f

Just don't get too comfortable force pushing. :) It's always good to make sure you're not on master, for example. :))

Once you've done that, you've got the latest commits, and no merge commits, etc.

laghee · 2018-05-18T15:48:55Z

That makes sense. I've always been suspicious of -f and avoided it, so now I understand why merging was always wonky.

Do I have to check out my remote master to do update and pull --rebase, or can I do that directly from the branch?

Edit: Answered my own question. 😄

* NPM Run Chrome Ammended * Added License, edited eslint commments, added space for readability * Changed windows to unix for eslint

miketaylr · 2018-05-18T16:12:00Z

Thank you!

miketaylr reviewed May 17, 2018

View reviewed changes

package.json Outdated

"web-ext": "^2.4.0",

"webpack": "^4.1.1",

"webpack-cli": "^2.0.10"

},

"license": "MPL-2.0"

"license": "MPL-2.0",

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

miketaylr reviewed May 17, 2018

View reviewed changes

tests/lib/helpers.js

@@ -2,11 +2,14 @@

* http://creativecommons.org/publicdomain/zero/1.0/ */

const webpack = require("webpack");

// import webpack from "webpack";

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

miketaylr reviewed May 17, 2018

View reviewed changes

tests/lib/helpers.js

/*

Returns a promise that resolves once webpack has compiled all the addon

configs have been built.

*/

// export default function

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

miketaylr reviewed May 17, 2018

View reviewed changes

miketaylr requested changes May 18, 2018

View reviewed changes

Kate Manning and others added 8 commits May 18, 2018 15:53

Issue 90. Problem - unexpected token import.

64e0e9d

Issue webcompat#90. Progress - Intern loader issue resolved.

619877b

Issue webcompat#90. Add Intern unit tests for isReportableUrl.

59ec825

Fixes 90. Add Intern unit tests for isReportableUrl.

c8699c8

NPM Run Chrome Ammended (webcompat#114)

d710d27

* NPM Run Chrome Ammended * Added License, edited eslint commments, added space for readability * Changed windows to unix for eslint

Issue webcompat#90. Add Intern unit tests for isReportableUrl.

b402716

Issue webcompat#90. Add Intern unit tests for isReportableUrl.

f7808e6

Issue webcompat#90. Add Intern unit tests for isReportableUrl.

63e2911

laghee force-pushed the issue90 branch from 03e5c7b to 63e2911 Compare May 18, 2018 16:06

miketaylr merged commit 643c022 into webcompat:master May 18, 2018

laghee deleted the issue90 branch May 22, 2018 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #90. Add Intern unit tests for isReportableUrl. #116

Fixes #90. Add Intern unit tests for isReportableUrl. #116

laghee commented May 17, 2018 •

edited

Loading

miketaylr commented May 17, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented May 18, 2018

laghee commented May 18, 2018 •

edited

Loading

miketaylr commented May 18, 2018

Fixes #90. Add Intern unit tests for isReportableUrl. #116

Fixes #90. Add Intern unit tests for isReportableUrl. #116

Conversation

laghee commented May 17, 2018 • edited Loading

miketaylr commented May 17, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented May 18, 2018

laghee commented May 18, 2018 • edited Loading

miketaylr commented May 18, 2018

laghee commented May 17, 2018 •

edited

Loading

laghee commented May 18, 2018 •

edited

Loading