Add an end to end test case. #134

f2prateek · 2017-11-15T00:36:36Z

No description provided.

f2prateek · 2017-11-15T00:41:30Z

This is a quick first pass so let me know if you have any suggestions.

I couldn't get await working here out of the box (Parsing error: The keyword 'await' is reserved), the code is a bit clunky.

Also if any of the callbacks fail, the test seems to hang forever, likely due to avajs/ava#567.

f2prateek · 2017-11-15T00:59:17Z

Given the number of external systems this calls out to, it'd be nice if there was a way to retry the test if a single run failed, and only fail if say 3/3 runs failed.

Not easily doable with AVA atm.
avajs/ava#934

stephenmathieson · 2017-11-15T01:30:37Z

test.js

+        .then(function (response) {
+          t.is(response.status, 200)
+
+          // TODO: fail and end the test if none of the requests match.


We could set a timeout on the client, which would cause an error and result in failure.

Not sure why the timeout on the client would help? The requests won't match if the library had a bug,api was down, etc.

Currently the test will just hang forever if this happens

I'm suggesting we call .end() after a specified amount of time. One way to do this would be to have Axios throw an error after N seconds.

So the async stuff fixed this!

stephenmathieson · 2017-11-15T01:31:14Z

test.js

+test.cb('e2e', t => {
+  const id = uid(16)
+
+  const testFn = function () {


If you make this an async function, you’ll be able to await promises.

I don't think this project is setup for async/await #134 (comment).

I tried:

const response = await axios.get(....)

And got the error: "Parsing error: The keyword 'await' is reserved".

Right, you need to make the testFn async:

const testFn = async function () { await somePromise() }

better:

const testFn = async () => { }

stephenmathieson · 2017-11-15T01:32:10Z

test.js

+    var axiosClient = axios.create({
+      baseURL: 'https://api.runscope.com',
+      timeout: 10 * 1000,
+      headers: { 'Authorization': 'Bearer ' + process.env.RUNSCOPE_TOKEN }


I’m worried about the implications this will have on open-source contributions, as folks won’t have access to this token.

I think the best we can do here is to run this only on CI? If we don't automate this I'm sure we'll forget to run this test before releasing/making big changes.

And I can't inline the token as it doesn't seem to be very well scoped. Though maybe we could setup a dedicated account for this and it wouldn't matter like we do for SauceLabs.

Perhaps moving this test out of the "normal" test suite and only having CI run it would solve the problem?

In package.json, we could do:

{ "scripts": { "test": "ava test.js", "test-e2e": "ava e2e/test.js } }

And in circle.yml:

test: override: - yarn test - yarn test-e2e

Instead of creating new folders and files, I propose we leave this test in test.js and use is-ci to determine whether to run it or not.

It's not about lines of code, but not doing something that's already been handled perfectly by other people. Left-pad argument doesn't apply anymore, since npm took measures to prevent situations like that in the future. See sindresorhus/ama#10 (comment) for a much more extensive answer to this.

It still doesn't make sense to me to to pull in 50+ lines of dependencies when a one-liner does the job for us. I understand the flexibility is-ci/ci-info will offer, but seems overkill if we don't need that.

But I'll put a pin in in this and use is-ci if that's what you'll want here.

As a compromise, I propose we ditch CI check altogether. Instead, let's use something more descriptive of what's actually happening:

if (process.env.INTEGRATION) { test(...) }

$ INTEGRATION=true npm test

There should probably be a better name than INTEGRATION too.

I very much prefer being explicit here and thought having to CI=true yarn test would have been awkward.

stephenmathieson · 2017-11-15T01:32:38Z

test.js

+
+    retries(axiosClient, { retries: 3 })
+
+    axiosClient.get('buckets/zfte7jmy76oz/messages?count=10')


What is “zfte7jmy76oz”?

It's the ID for our Runscope bucket. I'll link to it here.

Ahh ok, the a link would be great. 👍

Maybe a comment too?

stephenmathieson · 2017-11-15T01:33:11Z

test.js

+      id: `${id}`
+    }
+  }, function () {
+    setTimeout(testFn, 5000)


What is this timeout for?

Latency between sending to api.segment.io and it being received by an integration.

Gotcha. Maybe we should explain that in a comment?

Could it be shorter?

stephenmathieson · 2017-11-15T01:34:51Z

test.js

+  }, function () {
+    setTimeout(testFn, 5000)
+  })
+  analytics.flush()


Do we need this .flush()? It looks like we tell the library to flush after a single call.

Probably not, I didn't totally trust the library to do the right thing here so I added both.

I didn't totally trust the library

That's not good. Let's chat about that on Slack.

Maybe you should remove it to gain trust in the library? 😉

Yep I can change that.

I left the flush but removed the flushAt:1 part. Can reverse that if we want.

vadimdemedes · 2017-11-15T15:14:08Z

Given the number of external systems this calls out to, it'd be nice if there was a way to retry the test if a single run failed, and only fail if say 3/3 runs failed.

Not easily doable with AVA atm.

You can use the excellent async-retry for it.

vadimdemedes · 2017-11-15T15:15:26Z

test.js

@@ -447,3 +450,60 @@ test('alias - require previousId and userId', t => {
    })
  })
 })
+
+test.cb('e2e', t => {


You should make this test async and remove cb mode. All of the code in it can be converted to async/await.

vadimdemedes · 2017-11-15T15:15:37Z

test.js

+  const id = uid(16)
+
+  const testFn = function () {
+    var axiosClient = axios.create({


var => const

done.

Should this be a linter check?

vadimdemedes · 2017-11-15T15:17:21Z

test.js

+    var axiosClient = axios.create({
+      baseURL: 'https://api.runscope.com',
+      timeout: 10 * 1000,
+      headers: { 'Authorization': 'Bearer ' + process.env.RUNSCOPE_TOKEN }


headers: { 'Authorization': 'Bearer ' + process.env.RUNSCOPE_TOKEN } => headers: { authorization: `Bearer ${process.env.RUNSCOPE_TOKEN}` }

vadimdemedes · 2017-11-15T15:18:18Z

test.js

+    userId: 'prateek',
+    event: 'E2E Test',
+    properties: {
+      id: `${id}`


String is pointless here, can just be { id }

vadimdemedes · 2017-11-15T15:18:27Z

test.js

+    properties: {
+      id: `${id}`
+    }
+  }, function () {


Use arrow functions.

removed with the async stuff.

stephenmathieson · 2017-11-15T18:18:06Z

test.js

+
+  let found = false
+
+  messagesResponse.data.data.forEach(async item => {


We could make this a simple for loop and only iterate until we "find" what we're looking for:

const {data} = messagesResponse.data for (const item of data) { const response = await axiosClient.get(`buckets/zfte7jmy76oz/messages/${item.uuid}`) t.is(response.status, 200) const body = JSON.parse(response.data.data.request.body) if (id === body.properties.id) { found = true break } }

Yes, it should be that way, because forEach() doesn't care about async functions. Meaning, it will not wait until those functions finish.

stephenmathieson · 2017-11-15T18:18:22Z

test.js

+    const response = await axiosClient.get('buckets/zfte7jmy76oz/messages/' + item.uuid)
+    t.is(response.status, 200)
+
+    const body = JSON.parse(response.data.data.request.body)


I think Axios will return JSON for us if we ask it to.

How do I get it to do that? I tried:

const response = await axiosClient.get(`buckets/zfte7jmy76oz/messages/${item.uuid}`, { responseType: 'json' })

Try passing it in the options when creating axiosClient.

Still failed.

const axiosClient = axios.create({ baseURL: 'https://api.runscope.com', timeout: 10 * 1000, headers: { Authorization: `Bearer ${process.env.RUNSCOPE_TOKEN}` }, responseType: 'json' })

for (const item of messagesResponse.data.data) { const response = await axiosClient.get(`buckets/zfte7jmy76oz/messages/${item.uuid}`) t.is(response.status, 200) if (id === response.data.data.request.body.properties.id) { found = true break } }

> [email protected] test /Users/prateek/dev/src/github.com/segmentio/analytics-node > standard && ava 30 passed 1 failed end to end test /Users/prateek/dev/src/github.com/segmentio/analytics-node/test.js:491 490: 491: if (id === response.data.data.request.body.properties.id) { 492: found = true Rejected promise returned by test. Reason: TypeError { message: 'Cannot read property \'id\' of undefined', } Test.<anonymous> (test.js:491:18) step (test.js:52:191) npm ERR! Test failed. See above for more details.

Error is different here, properties is undefined.

stephenmathieson · 2017-11-15T18:21:33Z

test.js

@@ -447,3 +451,50 @@ test('alias - require previousId and userId', t => {
    })
  })
 })
+
+test('e2e', async t => {


I still think this should only run in certain conditions.

Maybe do something like:

const { RUN_E2E_TESTS } = process.env if (RUN_E2E_TESTS) { test('e2e', async t => { ... }) }

And in circle.yml, add RUN_E2E_TESTS=true in the "environment" config.

To avoid nesting, you can do this:

async function e2etest (t) { const id = uid(16) // ... } const { RUN_E2E_TESTS } = process.env if (RUN_E2E_TESTS) { test('e2e', e2etest) }

Yep, didn't get around to that yet. Will update on the original comment when I do.

Nesting here is totally fine, looks more natural.

@f2prateek could you also modify test title to be more descriptive?

Sure, do you have any other suggestions for naming?

send event to runscope

I updated the description and added some comments, let me know if that looks ok.

vadimdemedes · 2017-11-15T18:41:27Z

package.json

@@ -51,6 +51,7 @@
    "nsp": "^2.6.3",
    "pify": "^3.0.0",
    "sinon": "^2.3.8",
-    "standard": "^9.0.1"
+    "standard": "^9.0.1",
+    "await-sleep": "^0.0.1"


Use delay instead, it's high quality and widely used.

vadimdemedes · 2017-11-15T18:42:39Z

test.js

+  analytics.track({
+    userId: 'prateek',
+    event: 'E2E Test',
+    properties: {


=> properties: { id }

That should be a thing that should be flagged by the style guide/standard. If it didn't, i don't think it's worth bothering with those changes.

It's about consistency. Here it's on multiple lines and on the other ones it's on one (https://github.com/segmentio/analytics-node/pull/134/files#diff-1dd241c4cd3fd1dd89c570cee98b79ddR474, https://github.com/segmentio/analytics-node/pull/134/files#diff-1dd241c4cd3fd1dd89c570cee98b79ddR476).

I think that's okay. The point of code formatter is to not have to make these kinds of changes by hand. If a style guide doesn't flag it, I think it's okay to leave it as creative expression :)

(though I did update it here as I was making other changes at the same time).

vadimdemedes · 2017-11-15T18:42:55Z

test.js

+  const axiosClient = axios.create({
+    baseURL: 'https://api.runscope.com',
+    timeout: 10 * 1000,
+    headers: { 'Authorization': `Bearer ${process.env.RUNSCOPE_TOKEN}` }


'Authorization' => authorization (without quotes)

why's that? it should be capitalized?

HTTP headers are case insensitive.

vadimdemedes · 2017-11-15T18:48:09Z

test.js

+
+  let found = false
+
+  for (const item of messagesResponse.data.data) {


It doesn't look good tbh. Much better:

const requests = messagesResponse.data.data.map(async item => { const res = await axiosClient.get(`buckets/zfte7jmy76oz/messages/${item.uuid}`) return JSON.parse(res.data.data.request.body) }) const messages = await Promise.all(requests) const message = messages.find(message => message.properties.id === id) t.truthy(message)

Ref: https://paper.dropbox.com/doc/analytics-node-E2E-Test-9oavh3DFcFBXuqCJBe1o9

f2prateek · 2017-11-17T01:46:00Z

I updated some comments to better describe the test for non-Segment contributors.

I would like to add retries but blocked by the errors in #136. For now I'll merge this as is and can re-prioritize retries in the future if this ends up being a flaky test.

f2prateek force-pushed the e2e-test branch from 019ce95 to 360f27b Compare November 15, 2017 00:37

f2prateek requested review from vadimdemedes and stephenmathieson November 15, 2017 00:39

stephenmathieson reviewed Nov 15, 2017

View reviewed changes

vadimdemedes reviewed Nov 15, 2017

View reviewed changes

f2prateek force-pushed the e2e-test branch from 360f27b to ad732d3 Compare November 15, 2017 18:09

stephenmathieson reviewed Nov 15, 2017

View reviewed changes

f2prateek mentioned this pull request Nov 15, 2017

Ad e2e test retry #135

Closed

vadimdemedes reviewed Nov 15, 2017

View reviewed changes

f2prateek force-pushed the e2e-test branch from ad732d3 to 9a4eaab Compare November 15, 2017 18:41

vadimdemedes reviewed Nov 15, 2017

View reviewed changes

f2prateek force-pushed the e2e-test branch 3 times, most recently from 6483a5e to 72c1315 Compare November 15, 2017 18:56

f2prateek force-pushed the e2e-test branch 4 times, most recently from 307243e to 08fc33e Compare November 15, 2017 19:14

stephenmathieson approved these changes Nov 15, 2017

View reviewed changes

Add an end to end test case.

367b5d9

Ref: https://paper.dropbox.com/doc/analytics-node-E2E-Test-9oavh3DFcFBXuqCJBe1o9

f2prateek force-pushed the e2e-test branch from 08fc33e to 367b5d9 Compare November 17, 2017 01:42

f2prateek merged commit 1f930f5 into master Nov 17, 2017

f2prateek deleted the e2e-test branch November 17, 2017 01:46

rohitpaulk mentioned this pull request Dec 13, 2017

[TEST] Add an end-to-end test segmentio/analytics-ruby#137

Closed


		retries(axiosClient, { retries: 3 })

		axiosClient.get('buckets/zfte7jmy76oz/messages?count=10')


		let found = false

		messagesResponse.data.data.forEach(async item => {


		let found = false

		for (const item of messagesResponse.data.data) {

Add an end to end test case. #134

Add an end to end test case. #134

Conversation

f2prateek commented Nov 15, 2017 • edited by danielstjules Loading

f2prateek commented Nov 15, 2017

f2prateek commented Nov 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

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

vadimdemedes Nov 15, 2017 • edited Loading

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

vadimdemedes commented Nov 15, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f2prateek Nov 15, 2017 • edited Loading

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

f2prateek commented Nov 15, 2017 •

edited by danielstjules

Loading

vadimdemedes Nov 15, 2017 •

edited

Loading

vadimdemedes commented Nov 15, 2017 •

edited

Loading

f2prateek Nov 15, 2017 •

edited

Loading