Make the tests stable #239

pimterry · 2016-12-14T14:07:33Z

This fixes #237, building on top of #236.

This change is very simple, just fixes a Karma config bug (arguably a Karma bug, see karma-runner/karma#2505), brings in the new resin-request and puts balena-io-modules/balena-request#78 and balena-io-modules/balena-request#79 to use.

End result is that we now output some debug info during the tests about the requests being made, and retry failing requests. The debug output is just so we can debug this better if it does start to go wrong later - it's a bit noisy for now, but we can remove it soon once we're sure this change makes the tests stable.

Debug data is a short-term patch so we can debug potential (likely) later failures better - it's noisy, but it'll be useful. Retries are a medium-term fix, until we can work out why the tests are actually failing.

Before this, the tests ran on Linux, but not OSX/Windows (because the `env` preprocessor order isn't define, and it was valid to run it before Browserify, which apparently doesn't work).

jviotti · 2016-12-14T16:25:45Z

I see the following in the CI server logs:

{ Error: ENOENT: no such file or directory, open '.env'
  at Error (native)
  at Object.fs.openSync (fs.js:640:18)
  at Object.fs.readFileSync (fs.js:508:33)
  at Object.config (/home/travis/build/resin-io/resin-sdk/node_modules/dotenv/lib/main.js:30:37)
  at exports.loadEnv (/home/travis/build/resin-io/resin-sdk/tests/util.coffee:2:20)
  at module.exports (/home/travis/build/resin-io/resin-sdk/karma.conf.coffee:6:2)
  at Object.parseConfig (/home/travis/build/resin-io/resin-sdk/node_modules/karma/lib/config.js:377:5)
  at new Server (/home/travis/build/resin-io/resin-sdk/node_modules/karma/lib/server.js:56:20)
  at Object.exports.run (/home/travis/build/resin-io/resin-sdk/node_modules/karma/lib/cli.js:280:7)
  at Object.<anonymous> (/home/travis/build/resin-io/resin-sdk/node_modules/karma/bin/karma:3:23)
  at Module._compile (module.js:570:32)
  at Object.Module._extensions..js (module.js:579:10)
  at Module.load (module.js:487:32)
  at tryModuleLoad (module.js:446:12)
  at Function.Module._load (module.js:438:3)
  at Module.runMain (module.js:604:10)
  at run (bootstrap_node.js:394:7)
  at startup (bootstrap_node.js:149:9)
  at bootstrap_node.js:509:3
 errno: -2, code: 'ENOENT', syscall: 'open', path: '.env' }

Is that expected?

jviotti · 2016-12-14T16:27:09Z

Notice that the debug logs are currently causing the authentication token to be printed in the CI servers, which means that anyone looking can get access to our testing accounts.

emirotin · 2016-12-14T16:30:13Z

Can you edit the PR and make a PR to my branch please?

emirotin · 2016-12-14T16:31:33Z

I see the following in the CI server logs:

Heh, it's because of dotenv module. I didn't expect it to do so and expected it to silently do nothing if the env file is missing. And also I didn't see this message before.

emirotin · 2016-12-14T16:32:49Z

karma.conf.coffee

@@ -12,7 +12,7 @@ module.exports = (config) ->
 		captureConsole: true

 	karmaConfig.plugins.push(require('karma-env-preprocessor'))
-	karmaConfig.preprocessors['**/*.spec.coffee'] = [ 'env' ]
+	karmaConfig.preprocessors['**/*.spec.coffee'] = [ 'browserify', 'env' ]


pimterry · 2016-12-14T16:39:39Z

@emirotin Base branch changed.

@jviotti Yes, you're right, that's not good.

What should we do about this? Since it does initially look like this is now stable I'm no longer that worried about further debugging, so I could turn debugging off, and we can change the password. That doesn't totally solve the preexisting problem though - presumably anybody could submit a PR to this project that turns on debug, and immediately grab any new password we use?

jviotti · 2016-12-14T16:41:45Z

@pimterry PRs from forks will not have access to the secure environment variables we put in the CI servers, and we can safely trust people with write access to these repos (whose PRs can read the secure environment variables), since they are the resin.io team.

emirotin · 2016-12-14T16:45:13Z

Let's remove debug from this PR, merge it (it just goes to my branch), then figure out how to hide the token from the debug output.

emirotin · 2016-12-14T16:45:40Z

Ah, and also wrap the dotenev.config in try/catch probably?

emirotin · 2016-12-14T16:46:37Z

Hm, it shouldn't actually throw, it should be a warning, and

Dotenv outputs a warning to your console if missing a .env file. Suppress this warning using silent.
require('dotenv').config({silent: true});

pimterry · 2016-12-14T16:51:33Z

Debug removed, dotenv silenced (all these builds are a great way of testing if the tests really are stable now!)

pimterry · 2016-12-14T16:56:35Z

Also @jviotti you're totally right about the hiding secure vars thing for Travis, but the docs for Appveyor don't mention anything about that. They make it sound like it's all PRs or none for them, and you should only turn on vars in PRs for private repos: https://www.appveyor.com/docs/build-configuration/#secure-variables. Haven't tested though, is it just that the docs don't tell the whole story?

jviotti · 2016-12-14T17:02:30Z

@pimterry Looks like a docs-induced confusion :) I use secure env vars in Appveyor for some projects, and under settings, I see:

Which is exactly what we need.

emirotin · 2016-12-14T17:43:24Z

CI tests seem to be hung? No activity in Travis

pimterry · 2016-12-14T18:08:18Z

Not sure why Travis has hung, but it's queued in their infrastructure, not our code, so there's not much we can do. We're in the queue. There were a few pushes in quick succession for this, and for each one we seem to trigger two builds (the push on the branch and the PR itself), on both Travis & Appveyor, each of which runs two jobs (for two node versions), each of which runs the entire test suite for Node and the Browser, twice (because of prepublish). They're currently taking 50+ minutes, per job, so our backlog might take a while.

I could kill jobs for all commits but the very latest batch, but I quite want to let the builds all run, just to confirm they're definitely now stable.

I'm not sure why we're notifying on branches and PRs and duplicating these builds. Could we stop doing that?

It'd also be good to find a way to not run the whole 20 minute test suite twice with each build (as discussed on Flowdoc). Sounds like that's not practical for now, but we need to be a little careful - if the builds go from the current ~50 minutes to 60 then we'll hit the Travis time limit, and the builds won't run at all.

pimterry · 2016-12-14T18:44:34Z

Interestingly, one of the builds has failed: https://ci.appveyor.com/project/resin-io/resin-sdk/build/1.0.938/job/4w6t2nxcoxtcyg4g#L2414. No longer because of ETIMEDOUT, but because:

PhantomJS 2.1.1 (Windows 8 0.0.0) SDK Integration Tests given a logged in fresh user given a single application with two offline devices that share the same uuid root "before each" hook for "should be rejected with an error if there is an ambiguation between shorter uuids" FAILED
Request error: Data is referenced by uuid.
c:/Users/appveyor/AppData/Local/Temp/1/node_modules/resin-request/build/request.js:160:0

I'm pretty sure I actually know what this is: I'm running tests locally, and that test just failed randomly for me too, I think because it uses fixed device uuids.

Presumably device uuids must be unique across all devices, not just per-user? That would mean these tests will fail if anybody happens to be running those same few tests at the same time anywhere, which isn't great. Separate to the ETIMEDOUT issue, and unlikely to come up often normally, but still pretty messy.

emirotin · 2016-12-14T20:02:41Z

Yeah, tests are not isolated and it's not great at all. Ideally we'd like to run each of them against of a separate fresh DB :(
UUIDs should be globally unique, and it looks like the tests try to delete a record that's FKed by something else.

pimterry · 2016-12-14T20:23:07Z

UUIDs should be globally unique, and it looks like the tests try to delete a record that's FKed by something else.

Autogenerated ones will be, but the test explicitly provides fixed UUIDs: https://github.com/resin-io/resin-sdk/blob/af90835dee327bb450239bc768aeea56b09cdc6d/tests/integration.spec.coffee#L1129-L1130. I was assuming that that's why they're conflicting: both create the same device, and end up failing at some point because of the existing FK relationships (not sure where, maybe when you create new a device with an existing UUID but within a different application it explodes? Sounds like it should imo). Does that sound plausible?

I think we can find a way to isolate these though, at the cost of some extra complexity (assuming the above is right). I'll take a look tomorrow - let me know if I'm totally off track or if this isn't worth fixing.

emirotin · 2016-12-14T23:24:18Z

The "data is referenced" is when you try to delete as far as I remember, but maybe it's also thrown when you violate the unique constraint? @Page- any ideas?
Generally, we could just generate a unique session ID and prepend it to every unique field we inject into the DB.

I'll merge this one now into my branch.

Page- · 2016-12-15T00:43:56Z

The "data is referenced" is when you try to delete something that is referenced by a foreign key, as far as I'm aware that's the only case

pimterry added 2 commits December 14, 2016 13:22

Enable request retries and include debug data in test output

ef9a1f3

Debug data is a short-term patch so we can debug potential (likely) later failures better - it's noisy, but it'll be useful. Retries are a medium-term fix, until we can work out why the tests are actually failing.

Define Karma preprocessor order explicitly

fc99cb8

Before this, the tests ran on Linux, but not OSX/Windows (because the `env` preprocessor order isn't define, and it was valid to run it before Browserify, which apparently doesn't work).

pimterry requested review from Page-, emirotin and jviotti December 14, 2016 16:16

pimterry changed the base branch from master to sdk-browser December 14, 2016 16:30

emirotin reviewed Dec 14, 2016

View reviewed changes

Disable debug output, as it leaks secrets in CI

c25fdd0

pimterry force-pushed the 237-fix-test-timeouts branch from 6afc41d to c25fdd0 Compare December 14, 2016 16:48

Quieten dotenv, so it doesn't complain in CI

9c5d0c8

pimterry mentioned this pull request Dec 14, 2016

Update to support the new device registration flow #240

Merged

Page- approved these changes Dec 14, 2016

View reviewed changes

emirotin merged commit 2a420da into sdk-browser Dec 14, 2016

pimterry deleted the 237-fix-test-timeouts branch December 15, 2016 10:05

pimterry mentioned this pull request Dec 15, 2016

Make Resin-SDK builds gentler on the API #241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the tests stable #239

Make the tests stable #239

pimterry commented Dec 14, 2016 •

edited

Loading

jviotti commented Dec 14, 2016

jviotti commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin Dec 14, 2016

pimterry commented Dec 14, 2016

jviotti commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin commented Dec 14, 2016

pimterry commented Dec 14, 2016 •

edited

Loading

pimterry commented Dec 14, 2016

jviotti commented Dec 14, 2016

emirotin commented Dec 14, 2016

pimterry commented Dec 14, 2016

pimterry commented Dec 14, 2016

emirotin commented Dec 14, 2016

pimterry commented Dec 14, 2016

emirotin commented Dec 14, 2016

Page- commented Dec 15, 2016

Make the tests stable #239

Make the tests stable #239

Conversation

pimterry commented Dec 14, 2016 • edited Loading

jviotti commented Dec 14, 2016

jviotti commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin Dec 14, 2016

Choose a reason for hiding this comment

pimterry commented Dec 14, 2016

jviotti commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin commented Dec 14, 2016

emirotin commented Dec 14, 2016

pimterry commented Dec 14, 2016 • edited Loading

pimterry commented Dec 14, 2016

jviotti commented Dec 14, 2016

emirotin commented Dec 14, 2016

pimterry commented Dec 14, 2016

pimterry commented Dec 14, 2016

emirotin commented Dec 14, 2016

pimterry commented Dec 14, 2016

emirotin commented Dec 14, 2016

Page- commented Dec 15, 2016

pimterry commented Dec 14, 2016 •

edited

Loading

pimterry commented Dec 14, 2016 •

edited

Loading