add gosquared #14

craigmulligan · 2017-04-20T13:22:38Z

Connects to #13

This adds gosquared for browser and node.js, it keeps the modules current handling of non authenticated uses as mentioned in #12 (will handle that in a seperate PR).

Current issue is that I can't get the browser tests to pass for gosquared. I've tested it manually in a browser and it works but with karma the nock request is never fulfilled and I'm struggling to debug effectively.

The nockRequest.isDone() continues to return false, I'm confident the track method is being called.

Any ideas?

I saw something about xhr transport that's needed for the ga tests? Could it be related?

Change-type: minor

emirotin · 2017-04-20T20:48:01Z

The tests seem to be OK now?

emirotin · 2017-04-20T20:49:39Z

packages/resin-event-log/karma.conf.js

 	karmaConfig.sauceLabs = {
 		testName: packageJSON.name + ' v' + packageJSON.version
 	}
+	karmaConfig.browserConsoleLogOptions = {
+    level: 'log',


spaces/tabs

emirotin · 2017-04-20T20:50:11Z

packages/resin-event-log/package.json

@@ -7,7 +7,8 @@
    "bluebird": "^3.4.7",
    "lodash": "^4.0.0",
    "resin-mixpanel-client": "^2.1.0",
-    "resin-universal-ga": "^1.1.0"
+    "resin-universal-ga": "^1.1.0",
+    "resin-universal-gosquared": "*"


don't use *, use the proper semver range (like ^0.1.0)

emirotin · 2017-04-20T20:51:10Z

packages/resin-event-log/src/resin-event-log.js

@@ -11,7 +11,8 @@ var EVENTS = {
 	application: [ 'create', 'open', 'delete', 'osDownload' ],
 	environmentVariable: [ 'create', 'edit', 'delete' ],
 	device: [ 'open', 'rename', 'delete', 'terminalOpen', 'terminalClose' ],
-	deviceEnvironmentVariable: [ 'create', 'edit', 'delete' ]
+	deviceEnvironmentVariable: [ 'create', 'edit', 'delete' ],
+	page: [ 'view' ]


is visit a better name?

emirotin · 2017-04-20T20:51:50Z

packages/resin-event-log/src/adaptors/gosquared.js

+
+module.exports = function (options) {
+	var debug = options.debug,
+		gosquaredId = options.gosquaredId


missing comma means the following two vars are global

emirotin · 2017-04-20T20:52:04Z

packages/resin-event-log/src/adaptors/gosquared.js

+		apiKey = options.gosquaredApiKey
+		isBrowser = typeof window !== 'undefined'
+
+	if (!(gosquaredId)) {


unneeded extra parens

emirotin · 2017-04-20T20:53:07Z

packages/resin-event-log/src/resin-event-log.js

@@ -95,6 +97,7 @@ module.exports = function(options) {
 					hooks.afterCreate.call(_this, err, type, jsonData, applicationId, deviceId)
 				}).catch(function (err) {
 					// discard the hook error
+					console.log('OMGerr', err)


emirotin · 2017-04-20T20:53:36Z

packages/resin-event-log/test/resin-event-log.spec.js

@@ -114,6 +117,39 @@ function createGaMock(endpoint) {
 	return aggregateMock(mocks)
 }

+function validateGsQuery(event) {
+	return true


I was trying to get the test to pass by just returning straight away

emirotin · 2017-04-20T20:54:08Z

packages/resin-event-log/test/resin-event-log.spec.js

+		return true
+	}
+
+	if (!IS_BROWSER) {


unneeded check because of the previous guard

emirotin · 2017-04-20T20:54:46Z

packages/resin-event-log/test/resin-event-log.spec.js

+	if (!IS_BROWSER) {
+		return (
+			event.site_token === GOSQUARED_ID
+			&& event.api_key === GOSQUARED_API_KEY


better to keep the operator && on the prev line

craigmulligan · 2017-04-21T10:53:34Z

The tests were passing because I had altered them to not depend on the mocked request getting resolved.

Basically the xhr-mock is never hit :/ I checked the event function is fired and that the would be request matches the regex. I watching the monitoring it with wireshark but I'm not seeing anything coming from karma. Probably a fault of me not using wireshark correctly.

pimterry · 2017-04-21T14:15:11Z

Oh, bonus point on this @craig-mulligan: even without Chrome here, generally you don't want to use Wireshark for HTTP, it's way too complicated.

Try http://www.telerik.com/fiddler or https://www.charlesproxy.com/ - they're both HTTP proxies for testing. Way way easier for HTTP stuff, and also have built in support for things like MitM inspection on HTTPS, and header and json parsing etc.

craigmulligan · 2017-04-24T10:34:12Z

@pimterry, @emirotin I got to the bottom of it (sort of)

Tests were really hard to write, because of series of peculiarities with the go squared tracking script.

First go squared doesn’t make xhr requests it instead loads the requests by adding a script tag with the endpoint as the src to the for each request. Which means I couldn’t access it from the test script.

Phantom.js does have a onResourceRequested api hook which can tell me when resource was called but it only ever fires for the first request made to gosquared, some others have hit this too (if I use window.performace.getEntriesByType('resource') in chrome I can see all the requests. Unfortunately phantom.js doesn't have that browser api implemented.

Secondly gosquared it only triggers an event request after a page view is called. And thirdly it randomises it’s hostname, between data2.gosquared and data.gosquared. Which made request tracking a mission.

Anyway, I've implemented some checks which use the onResourceRequested hook I mentioned, although currently all the browser tests do is check that a request was made to gosquared.com unfortunately without phantom.js onResourceRequested working properly I can’t get much further.

Any better ideas on ways to test this?

I’ve asked gosquared for a debug version of tracker.js that sends xhr requests, but no response yet.

craigmulligan · 2017-04-27T08:15:22Z

Ping @emirotin

pimterry · 2017-04-27T16:24:33Z

@craig-mulligan Damn, that's going to be very annoying.

I'm feeling like the current mocking code has caused us a few problems, because so many of these libraries do strange things. Should we try refactoring this to a different approach? Can we set up an actual proxy server, and then point PhantomJS/Node to that when we start it up, and use that to intercept and check all the requests? Should be drastically more reliable and easier to debug. Potentially slightly slower and a little more complicated internally, but could be worth it imo, if it's possible.

I've got a basic implementation of programmable proxy server for testing with Node from a while back, which might be useful if we do want to try this: https://www.npmjs.com/package/http-server-mock. It's mostly stubbing right now, rather than mocking, but you could extend it fairly easily. No docs, but there's a bunch of usage for AWS stubbing that should cover the current API, e.g: https://github.com/pimterry/dev-bot-tool/blob/master/test/integration/aws-deploy-test.ts#L30 + https://github.com/pimterry/dev-bot-tool/blob/master/test/helpers/aws-mocks.ts#L8-L33.

The tricky bit is likely to be setting up, tearing down and communicating with a proxy server from inside the tests. I'm not totally sure how that'd work off the top of my head, but I'm pretty sure it's all possible. Other ideas welcome too!

craigmulligan · 2017-05-01T13:42:33Z

Hey @pimterry I've been using this http://www.mock-server.com/ for a personal project I haven't touched the verification much but looks pretty http://www.mock-server.com/proxy/verification.html.It's written in Java :/ but has a javascript client to start/stop and add expectations.

What do you think?

pimterry · 2017-05-02T16:58:03Z

@craig-mulligan that looks ok in principle, but I'd much rather we didn't have to depend on Java... Getting everybody who wants to develop this to have to install Java and get this set up will be annoying. Doesn't look like their Node module handles server startup either, only communication, so you'll need a Node script that's going to have to reliably manage a Java process in a neat cross platform way, and it's all just going to get nasty.

I'd be strongly tempted to just build one, either extending http-server-mock or otherwise. You just need a Node HTTP server that exposes a JSON api for mock configuration & verification, to start it up before the test suite, and to point the browser at that as a proxy. Should be relatively easy I think. Do you want to try putting together a small proof of concept of that inside this repo to get this working, and then we can look at building it into something more substantial and reliable once we've got these tests working and this PR merged?

craigmulligan · 2017-05-02T17:07:17Z

@pimterry yea I just had a go at implementing the http://www.mock-server.com/. It has a client ect so you could do everything except the client isn't isomorphic so browsers tests fail.

A basic implementation would be a great help! Here is a script I was using to start the mock server before the tests.

craigmulligan · 2017-05-08T08:39:04Z

@pimterry I've removed the browser tests. Should be good for review.

pimterry

Looks good to me, but make sure @emirotin's happy too before we merge this.

craigmulligan · 2017-05-12T09:03:43Z

Thanks Tim, @emirotin could you take another look?

emirotin · 2017-05-15T07:38:07Z

packages/resin-event-log/src/adaptors/gosquared.js

+		logout: function() {
+			return gsClient.logout()
+		},
+		track: function (prefix, type, data) {


why ignoring prefix?

Good catch. I now pass them down like we do for mixpanel so events are named like so: [ prefix ] eventType.

emirotin · 2017-05-15T07:39:34Z

packages/resin-event-log/test/resin-event-log.spec.js

@@ -114,8 +116,33 @@ function createGaMock(endpoint) {
 	return aggregateMock(mocks)
 }

+function validateGsQuery(event) {


event sounds misleading, it's the queryString, is it?

emirotin · 2017-05-15T07:40:26Z

packages/resin-event-log/test/resin-event-log.spec.js

+		filterBody: function() { return true },
+		response: '1'
+	})
+	delete options.event


What is event, how does it end up in the options (I don't see it ever passed there) and why do we delete it?

Ah I was passing the expected event to check if it was in the querystring, because the browser passes it that way. But in node it passes it as the request body, I didn't clean up properly when removing the browser tests.

emirotin · 2017-05-15T07:42:02Z

packages/resin-event-log/test/resin-event-log.spec.js

+						expect(mockedRequest.isDone()).to.be.ok
+						done()
+					} else {
+						// Don't mock browser tests see:


This needs a big TODO

emirotin · 2017-05-15T07:43:32Z

packages/resin-universal-gosquared/src/gs-loader.js

+	window,
+	document,
+	'script',
+  'https://d1l6p2sc9645hc.cloudfront.net/tracker.js',


Different indent

emirotin · 2017-05-15T07:45:27Z

packages/resin-universal-gosquared/src/node.js

+		},
+		track: function(type, data) {
+			// if called before `login` create the object with the random ID
+			createVisitor()


Currently this will recreate the visitor. It will also drop the userId link which sounds wrong to me

Yes, that's wrong I need a if (visitor) return visitor in createVisitor.

craigmulligan · 2017-05-15T10:49:28Z

@emirotin could you re-review :)

emirotin · 2017-05-15T21:55:05Z

packages/resin-universal-gosquared/src/node.js

+		})
+
+		if (userId) {
+			gosquared.createPerson(userId)


These lines were different previously reading visitor = gosquared.createPerson(userId). Is it equivalent? Also I didn't find this method on the gosquared docs site

Yea, if you don't identify a user with .createPerson() the lib will handle it for you. So it think it's simpler to only assign a user if an id is provided.

See the on this example on this page it should clarify.

What I'm saying is that you're not assigning the result of createPerson, the linked page doesn't make it clear to me this will have any effect.

Ah I see what you mean... and after looking at the source you are ofc right, it's a factory and does not update the instance.

craigmulligan · 2017-05-16T11:15:08Z

@emirotin I went through it again after your last comment and found another bug where I didn't actually pass the user.id to go squared lib. I didn't catch with manual testing because i was using the browser and it was automatically generating ids.

I've fixed it and added a test case for node.

emirotin · 2017-05-16T11:33:02Z

Would this be captured by node tests? Can we add some checks, like inside of the tracking verify that the proper user ID has been used?

emirotin · 2017-05-16T11:34:21Z

packages/resin-event-log/test/resin-event-log.spec.js

+				filterBody: function (body, eventName) {
+					return (
+						body.event.name === '[' + SYSTEM + '] ' + EXPECTED_EVENT &&
+						body.person_id == FAKE_USER.id


NIT but can be strict comparison for consistency

It can't because user.id is a number and the body.person_id is a string. I can do explicit type conversion but implicit is better in this case no?

emirotin

LGTM but if you can improve the tests please do before merging.
Also please don't forget to squash the commits

craigmulligan · 2017-05-16T12:24:57Z

@emirotin cool, what do you have in mind for improving the tests?

emirotin · 2017-05-16T12:30:08Z

See my previous comment about catching the error you didn't notice with manual testing:

Would this be captured by node tests? Can we add some checks, like inside of the tracking (mock) verify that the proper user ID has been used?

craigmulligan · 2017-05-16T12:51:05Z

yes, I've added a test case for that for gosquared. I'll see what effort is required for the other services.

emirotin · 2017-05-16T12:52:04Z

oh, I didn't notice, I think it's good to merge then, if you can send a separate PR for other services - awesome :), but it doesn't have to block this one

Connects to #13 Change-type: minor

craigmulligan requested review from emirotin and pimterry April 20, 2017 13:22

emirotin reviewed Apr 20, 2017

View reviewed changes

emirotin suggested changes Apr 20, 2017

View reviewed changes

pimterry mentioned this pull request May 5, 2017

Move mocking to use a full proxy server #16

Open

pimterry approved these changes May 9, 2017

View reviewed changes

emirotin reviewed May 15, 2017

View reviewed changes

emirotin suggested changes May 15, 2017

View reviewed changes

craigmulligan force-pushed the 13-add-gosquared branch from 7230376 to 04c6a22 Compare May 15, 2017 10:25

emirotin reviewed May 15, 2017

View reviewed changes

emirotin reviewed May 16, 2017

View reviewed changes

emirotin approved these changes May 16, 2017

View reviewed changes

craigmulligan changed the title ~~[WIP] add gosquared~~ add gosquared May 16, 2017

add gosquared

a907a9a

Connects to #13 Change-type: minor

craigmulligan force-pushed the 13-add-gosquared branch from 93df1a4 to a907a9a Compare May 16, 2017 15:06

craigmulligan merged commit dd31e0d into master May 17, 2017

add gosquared #14

add gosquared #14

Conversation

craigmulligan commented Apr 20, 2017

emirotin commented Apr 20, 2017

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

craigmulligan commented Apr 21, 2017 • edited

pimterry commented Apr 21, 2017

craigmulligan commented Apr 24, 2017

craigmulligan commented Apr 27, 2017

pimterry commented Apr 27, 2017

craigmulligan commented May 1, 2017

pimterry commented May 2, 2017

craigmulligan commented May 2, 2017

craigmulligan commented May 8, 2017

pimterry left a comment

Choose a reason for hiding this comment

craigmulligan commented May 12, 2017

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

craigmulligan May 15, 2017 • edited

Choose a reason for hiding this comment

craigmulligan commented May 15, 2017

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

craigmulligan commented May 16, 2017

emirotin commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emirotin left a comment

Choose a reason for hiding this comment

craigmulligan commented May 16, 2017

emirotin commented May 16, 2017

craigmulligan commented May 16, 2017

emirotin commented May 16, 2017

craigmulligan commented Apr 21, 2017 •

edited

craigmulligan May 15, 2017 •

edited