Unit test "page-detect" module #148

DrewML · 2016-04-03T20:40:38Z

@sindresorhus @paulmolluzzo @hkdobrev Still ~~heavily~~ working on this, but I wanted to start getting feedback sooner rather than waiting until I'm done.

Few things I'm looking for comments/discussion on:

Proper (or not) usage of ava
Whether I should keep or scrap helpers like urisMatch. Benefit being less code duplication, downside being stack-traces with a helper on top, and some shared logic between tests
window-mocking strategy. I was hoping there was an npm module that would take care of this, but I didn't find a suitable one. Let me know if you've seen one. I could probably use JSDom, but it seems a bit heavy for this particular use-case (and I suspect it would actually make requests to the page you set as location.href). Another option would be to change the page-detect module to have each public fn accept the arg for the current location.
How do we want to handle the tests that hit the DOM (isRepoRoot, isCommit)?
Should I use t.plan(ava feature)?

TODO:

Test some URIs that shouldn't match for each predicate method (isGist, isDashboard, etc)

hkdobrev · 2016-04-05T20:28:56Z

extension/page-detect.js

-	const isGistCheck = location.hostname === 'gist.github.com';
-
-	const isGist = () => isGistCheck;
+	const isGist = () => location.hostname === 'gist.github.com';


@DrewML I did this like that, because isGist() is called for every isRepo() call which is called for every other check. But location.hostname can't change without invalidating the JS context, so it's safe to check it only once.

Happy to change it if we can't test it otherwise though.

Yeah, I changed it for the purposes of making testing a tad bit easier. We technically can test without changing it, but it requires deleting a reference from node's require.cache and re-requiring it each time we need to make a change to location.hostname, which doesn't seem like a great thing to be doing in tests if it can be avoided.

@hkdobrev Doesn't really matter. The overhead of one function is negligible.

sindresorhus · 2016-04-11T08:56:26Z

tests/page-detect.spec.js

+const {pageDetect} = window;
+
+const urisMatch = (t, fn, testURIs = []) => {
+	testURIs.forEach(uri => {


Use a for...of loop.

sindresorhus · 2016-04-11T09:00:31Z

Looks good! :)
Keep it. The less duplication the better. We're actually working on introducing test macros to make this kind of thing simpler: [Idea]: test macros avajs/ava#695 (Thoughts welcome there)
What you did looks fine.
I don't see the point in overcomplicating it. I would just leave them out.
Nah. Really no point in our case. See: https://github.com/sindresorhus/ava/blob/master/docs/recipes/when-to-use-plan.md

sindresorhus · 2016-04-11T09:01:34Z

package.json

  },
  "devDependencies": {
+    "ava": "^0.13.0",


"ava": "*",

I prefer using * in my own things to always be on latest so I can catch error before my users do.

DrewML · 2016-04-13T00:11:10Z

Huh - I've been using Babel for so long, I didn't realize that Node v5 still didn't support destructing assignment :(

@sindresorhus What do you want to do for this scenario? I think our options are:

Don't use ES2015 features in any files under test
Use babel-register in ava's require option, with the es2015 node 5 preset (or just the explicit plugins we need, as an alternative to the preset)
??

sindresorhus · 2016-04-13T00:15:46Z

2

DrewML · 2016-04-13T00:50:14Z

Updates pushed.

paulmolluzzo · 2016-04-13T01:01:51Z

test/page-detect.js

+
+test('isDashboard', t => {
+	urisMatch(t, pageDetect.isDashboard, [
+		'https://github.com/',


https://github.com/dashboard should also match.

and https://github.com/dashboard and https://github.com.

sindresorhus · 2016-04-13T01:01:55Z

One minor dumb nit, but URI => URL

paulmolluzzo · 2016-04-13T01:14:40Z

@DrewML This is excellent, well done! Adds a lot of confidence to the URL regexes we rely on.

Just a general question about the pattern of match/don't match: Many URLs also work with the full/partial commit hash (commits and tags come to mind). Do we care to create "versions" of these in the match/don't match to make sure we are testing against the lowest and highest amount?

For example, these all "count", are identical in content, and the tests pass if I add them to the 'isCommitList' test:

https://github.com/sindresorhus/refined-github/commits/0.13.0
https://github.com/sindresorhus/refined-github/commits/230c2
https://github.com/sindresorhus/refined-github/commits/230c2935fc5aea9a68
https://github.com/sindresorhus/refined-github/commits/230c2935fc5aea9a681174ddbeba6255ca040d63

Do we care to include this stuff or is that just being overkill?

Either way, this is excellent and passing for me. (Node v4.4.0 and npm 3.8.0)

LGTM 🚀

paulmolluzzo · 2016-04-13T01:24:02Z

Sorry, another realization: There's no coverage of isRepoRoot.

DrewML · 2016-04-13T01:27:18Z

@paulmolluzzo See 4 here and in the first comment. We're leaving out tests which rely on the DOM (for now), but we could always revisit.

DrewML · 2016-04-13T01:27:44Z

Also, re: the shortened URIs, I think it's worth testing (Good catch). I'll push an update.

paulmolluzzo · 2016-04-13T01:29:38Z

And I don't see coverage of isRepoTree or isSingleFile, neither of those use the DOM though isSingleFile uses the ownername and reponame from getOwnerAndRepo that is also not tested.

DrewML · 2016-04-13T01:30:39Z

Ah yes, good call again - just merged isSingleFile in from PR #154 when addressing merge conflicts.

DrewML · 2016-04-13T04:42:20Z

Updates pushed :)

DrewML · 2016-04-13T06:41:14Z

test/page-detect.js

+
+	Object.keys(ownerAndRepo).forEach(url => {
+		location.href = url;
+		t.deepEqual(ownerAndRepo[url], pageDetect.getOwnerAndRepo());


Note: I updated ava to * in package.json in a recent commit - if t.deepEqual fails, run npm install to update to the latest version

paulmolluzzo · 2016-04-14T00:51:45Z

What about isRepoTree? Could we get a test for that in here too? (No DOM test in that.)

Still passing and looking good otherwise.

DrewML · 2016-04-14T04:14:53Z

Haha, how did I miss so many!? 😆 Updates pushed @paulmolluzzo

paulmolluzzo · 2016-04-15T01:29:02Z

OK, I'm good with this.

Any last thoughts @hkdobrev or @sindresorhus?

sindresorhus · 2016-04-15T10:34:14Z

Terrific work on this @DrewML. Feels good having some tests :)

DrewML mentioned this pull request Apr 5, 2016

Add "Copy" button for raw file contents in single file view #154

Merged

hkdobrev reviewed Apr 5, 2016
View reviewed changes

sindresorhus force-pushed the master branch from 5fa7607 to bb7e24b Compare April 6, 2016 18:51

sindresorhus reviewed Apr 11, 2016
View reviewed changes

Andrew Levine added 2 commits April 12, 2016 19:00

Working on unit-testing page-detect module

1e27d79

more tests

68ee8d5

Code review updates, and node 5.x compatibility

e1aba6f

DrewML force-pushed the ava branch from 5dfb471 to e1aba6f Compare April 13, 2016 00:22

Test uris that should *not* match

e3628fe

paulmolluzzo reviewed Apr 13, 2016
View reviewed changes

sindresorhus changed the title ~~[WIP] Unit Test "page-detect" module~~ Unit Test "page-detect" module Apr 13, 2016

Code review updates

61cb60e

Updates based on feedback

4ea33a4

DrewML reviewed Apr 13, 2016
View reviewed changes

Good thing GitHub has a rebase option in the UI now :)

9ec4a57

sindresorhus changed the title ~~Unit Test "page-detect" module~~ Unit test "page-detect" module Apr 15, 2016

sindresorhus merged commit 0bfd735 into master Apr 15, 2016

sindresorhus deleted the ava branch April 15, 2016 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit test "page-detect" module #148

Unit test "page-detect" module #148

DrewML commented Apr 3, 2016

hkdobrev Apr 5, 2016

hkdobrev Apr 5, 2016

DrewML Apr 5, 2016

sindresorhus Apr 11, 2016

sindresorhus Apr 11, 2016

sindresorhus commented Apr 11, 2016

sindresorhus Apr 11, 2016

DrewML commented Apr 13, 2016

sindresorhus commented Apr 13, 2016

DrewML commented Apr 13, 2016

paulmolluzzo Apr 13, 2016

sindresorhus Apr 13, 2016

sindresorhus commented Apr 13, 2016

paulmolluzzo commented Apr 13, 2016

paulmolluzzo commented Apr 13, 2016

DrewML commented Apr 13, 2016

DrewML commented Apr 13, 2016

paulmolluzzo commented Apr 13, 2016

DrewML commented Apr 13, 2016

DrewML commented Apr 13, 2016

DrewML Apr 13, 2016

paulmolluzzo commented Apr 14, 2016

DrewML commented Apr 14, 2016

paulmolluzzo commented Apr 15, 2016

sindresorhus commented Apr 15, 2016

Unit test "page-detect" module #148

Unit test "page-detect" module #148

Conversation

DrewML commented Apr 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

Choose a reason for hiding this comment

sindresorhus commented Apr 11, 2016

Choose a reason for hiding this comment

DrewML commented Apr 13, 2016

sindresorhus commented Apr 13, 2016

DrewML commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Apr 13, 2016

paulmolluzzo commented Apr 13, 2016

paulmolluzzo commented Apr 13, 2016

DrewML commented Apr 13, 2016

DrewML commented Apr 13, 2016

paulmolluzzo commented Apr 13, 2016

DrewML commented Apr 13, 2016

DrewML commented Apr 13, 2016

Choose a reason for hiding this comment

paulmolluzzo commented Apr 14, 2016

DrewML commented Apr 14, 2016

paulmolluzzo commented Apr 15, 2016

sindresorhus commented Apr 15, 2016