Re-inject native Node modules #4970

mjesun · 2017-11-27T16:09:06Z

This PR improves the sandboxing for native modules. Every time a native require is detected, Jest will:

introduce it inside the sandbox by re-evaluating its source code, exposed through process.binding('natives'); which differs from the previous approach where require(moduleName) was done on the main context; and
switch the require logic to know it's inside a native module (thus, internal/util will not resolve to the NPM internal module, but to the native Node one).

This kills all memory leaks related to process and native module modification. I have added an integration test with the most common use cases. Executing the test (./jest leak_detection) on a previous commit will fail because of detected leaks.

It also makes the jest test suite leak free itself (tried with ./jest -i --detectLeaks packages).

SimenB · 2017-11-27T16:12:49Z

Does this fix #4630?

mjesun · 2017-11-27T16:14:54Z

Nope; but I'll work on the child one as soon as I can. I'm very curious to see why this test breaks 🙂

thymikee · 2017-11-27T16:19:02Z

packages/jest-jasmine2/src/index.js

+  // reference after adding some variables to it; but you still need to pass
+  // it when calling "jasmine2".
+  if (!environment) {
+    throw new ReferenceError('Please pass a valid environment object');


- valid environment object + valid Jest Environment

thymikee · 2017-11-27T16:22:47Z

packages/jest-jasmine2/src/index.js

@@ -85,12 +92,21 @@ async function jasmine2(
    if (config.resetMocks) {
      runtime.resetAllMocks();

+      if (!environment) {
+        throw new ReferenceError('Environment should be cleaned at the end');
+      }


Can this be more descriptive?

thymikee · 2017-11-27T16:26:21Z

packages/jest-runtime/src/index.js

+      moduleRegistry = this._moduleRegistry;
+    }
+
+    let modulePath;


Can we move this to the top?

thymikee · 2017-11-27T16:54:19Z

packages/jest-runtime/src/index.js

+    switch (moduleName) {
+      case 'buffer': // Causes issues when passing buffers to another context.
+      case 'module': // Calls into native_module, which is not mockable.
+      case 'os': // Pure native module.


How do we maintain this whitelist?

This way 😄 . There is no way of knowing that os is a native module, and that module cannot be re-evaluated. Essentially trial/error.

Actually, os module should probably be wrapped with an object on top. Once we get this merged I will open a task for that.

Ok, so I did a bunch of things in the last commit:

I added a test suite that requires all native modules, so this will help us to semi-automate this list.

I also deep copy os and v8 modules so that they are sandboxed as well (i.e. if someone modifies them, they won't leak).

And finally I fixed the V8 crash for Node 8 (async_hooks can't be re-required).

thymikee · 2017-11-27T16:56:20Z

packages/jest-util/src/__tests__/create_process_object.test.js

 import createProcessObject from '../create_process_object';

 it('creates a process object that looks like the original one', () => {
  const fakeProcess = createProcessObject();

  // "process" inherits from EventEmitter through the prototype chain.
-  expect(fakeProcess instanceof EventEmitter).toBe(true);
+  expect(typeof fakeProcess.on).toBe('function');


So fakeProcess instanceof EventEmitter returns false now? Maybe worth to update the comment.

This is unfortunate, but yes. Because EventEmitter is re-evaluated now, it is not the parent EventEmitter anymore. In order to get that check right we should build the process object inside the VM context. However, this requires being able to do require('events') which overcomplicates all the current flow.

The comment is still valid, since we check the existence of the on & various methods; but I can clarify it.

SimenB · 2017-11-27T19:50:18Z

integration_tests/__tests__/__snapshots__/failures.test.js.snap

@@ -314,6 +316,8 @@ exports[`works with node assert 1`] = `
    Message:
      Missing expected exception.

+      at assert:1:1


we don't want this in the stack at all, do we?

Since these modules are now re-injected, they will appear in the stack. I don't think it's critical, though.

it's not useful though, so why not remove it? ref #4695

How do you decide what comes from an internal and what not? Notice that now internals are not internals anymore, because we re-evaluate them 😉

IMO internal === jest or node core

Yeah, but I'm unsure on how can we remove an arbitrary number of stack frames from the stack. How are we doing that now? I can maybe adapt my filenames to the existing way.

it's cleaned in this function: https://github.com/facebook/jest/blob/9043a44fca5b5e4880aed3579ab0c56775ec686b/packages/jest-message-util/src/index.js#L121-L163

Yeah these guys have to go. We need a way to annotate node libs so they don't come up in stack traces.

@SimenB this looks promising! I can inject native modules with native on the file name, and I think I can get rid of them.

thymikee · 2017-11-27T19:54:01Z

packages/jest-runtime/src/index.js

+      case 'os': // Pure native module.
+      case 'v8': // Contains invalid references.
+        // $FlowFixMe: dynamic require needed.
+        localModule.exports = deepCyclicCopy(require(moduleName));


Does it have a noticeable impact on perf? And are we sure there's nothing to blacklist there?

I checked both; v8 is just a set of functions, and os has some additional properties (os.constants), but it simple enough to be copied.

SimenB · 2017-11-27T20:04:24Z

integration_tests/__tests__/require_all_modules.test.js

+  const result = runJest.json('require-all-modules').json;
+
+  if (!result.success) {
+    console.warn(result);


for debugging? should it be removed?

I added it so that if a module throws when required, we can know who was based on the console 🙂

SimenB · 2017-11-27T20:06:11Z

jest-inspect

@@ -0,0 +1,9 @@
+#!/usr/bin/env node --inspect-brk


SimenB · 2017-11-27T20:07:05Z

packages/jest-jasmine2/src/index.js

  runtime: Runtime,
  testPath: string,
 ): Promise<TestResult> {
+  // The "environment" parameter is nullable just so that we can clean its


will this help pinpointing #4710?

SimenB · 2017-11-27T20:43:19Z

Failing on appveyor 🙁

mjesun · 2017-11-27T20:51:22Z

Yes, I just debugged it. What is happening is that jasmine-reporters/src/appveyor_reporter.js is injected inside the VM context (see here), but it is called after the test finishes. By then, the environment has been cleaned and the reporter fails.

To be precise, what fails is the execution of any require within the VM context after the test suite has finished. The appveyor_reporter.js has an inline require to http, which is the one breaking.

IMO this reporters should be injected outside the VM context (i.e. on the main context); but I wonder if doing so can create other undesirable side effects. cc @cpojer, @SimenB, @thymikee.

thymikee · 2017-11-27T22:07:35Z

I guess it makes sense to create another hook which works outside of VM context and put jasmine reporters there.

cpojer · 2017-11-28T08:12:17Z

Yeah, we'll need to fix that issue first before we can merge this PR.

cpojer · 2017-11-28T08:13:53Z

integration_tests/__tests__/failures.test.js

@@ -14,7 +14,8 @@ const runJest = require('../runJest');

 const dir = path.resolve(__dirname, '../failures');

-const normalizeDots = text => text.replace(/\.{1,}$/gm, '.');
+const normalize = text =>
+  text.replace(/\.{1,}$/gm, '.').replace(/\bassert:\d+:\d+/g, 'assert:1:1');


what is this change for?

assert can change from one version to another, and since now we re-inject the native module, line numbers can change, thus not making the snapshot to match. This ensures that line numbers are transformed into something consistent across Node versions.

Since I'm going to try make them disappear, that doesn't make sense anymore.

cpojer · 2017-11-28T08:16:45Z

integration_tests/require-all-modules/test.js

+ */
+
+it('requires all native modules to check they all work', () => {
+  Object.keys(process.binding('natives'))


If in a version of node process.binding changes to not return anything, this test will still pass but the sandboxing is broken. Can we change this to make sure it requires the right modules?

Everything would be broken then, because we rely on process.binding already (e.g. is_builtin_module from jest-resolve 😄

What I'm going to do is to check that the length is greater than the current amount of modules for Node 6, which should be good signaling.

Yep, I'm fine with that. If they ever remove core modules our test will fail, but that's fine and something we'd want to know about anyway.

cpojer · 2017-11-28T08:18:26Z

packages/jest-jasmine2/src/index.js

@@ -25,10 +25,17 @@ const JASMINE = require.resolve('./jasmine/jasmine_light.js');
 async function jasmine2(
  globalConfig: GlobalConfig,
  config: ProjectConfig,
-  environment: Environment,
+  environment: ?Environment,


woah, let's not do this please. For example by renaming the environment variable below?

We have to; I need to kill all environment references.

cpojer · 2017-11-28T08:18:56Z

packages/jest-jasmine2/src/index.js

@@ -85,12 +92,24 @@ async function jasmine2(
    if (config.resetMocks) {
      runtime.resetAllMocks();

+      if (!environment) {
+        throw new ReferenceError(


Is there any way that this can actually happen? How about just doing `if (environment && config.timers === 'fake') { … } ?

cpojer · 2017-11-28T08:19:18Z

packages/jest-jasmine2/src/index.js

      if (config.timers === 'fake') {
        environment.fakeTimers.useFakeTimers();
      }
    }
  });

+  // Free references to environment to avoid leaks.
+  env.afterAll(() => {
+    environment = null;


Who is holding on to environment?

The beforeAll closure, through environment.fakeTimers.useFakeTimers().

cpojer · 2017-11-28T08:19:32Z

packages/jest-runner/src/run_test.js

@@ -142,6 +143,11 @@ async function runTestInternal(
    });
  } finally {
    await environment.teardown();
+    await runtime.teardown();
+
+    // Free references to enviroment to avoid leaks.


environment.

cpojer · 2017-11-28T08:22:18Z

packages/jest-util/src/__tests__/create_process_object.test.js

 import createProcessObject from '../create_process_object';

 it('creates a process object that looks like the original one', () => {
  const fakeProcess = createProcessObject();

-  // "process" inherits from EventEmitter through the prototype chain.
-  expect(fakeProcess instanceof EventEmitter).toBe(true);


Hah, I think this is a bug actually. You are creating the process object with EventEmitter from the parent but it should use runtime._requireNativeModule('events') so that this test still passes. Changing this test isn't the right fix here imho.

Exactly; I explained that to @thymikee before. The issue here is that in order to get the child EventEmitter I need to execute in the context require('events').EventEmitter. Unfortunately at this point neither the context, nor the require implementation are ready; so it's too complex to add the change in this PR.

Since I just re-did the process object about a week ago, and before it did not inherit from EventEmitter either, I think we are more or less safe to do this.

However, at the core of the issue, you are both right, the check should pass 🙂

In that case let's change where we are initializing process then? It seems like that should happen later or lazily. You could make process a lazy getter that, when first accessed, overwrites itself by doing global.process = process. What do you think about that? We just need to make sure there is nothing in events or its dependencies that depends on process which would produce a circular dependency.

I tried making this work, but I ended up with a V8 crash (see below). I think we should revisit it later; but it should be kept out of the scope of this PR.

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. 1: node::Abort() [/Users/.../.nvm/versions/node/v8.9.1/bin/node] 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/.../.nvm/versions/node/v8.9.1/bin/node] 3: v8::V8::ToLocalEmpty() [/Users/.../.nvm/versions/node/v8.9.1/bin/node] 4: node::(anonymous namespace)::ContextifyScript::RunInContext(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/.../.nvm/versions/node/v8.9.1/bin/node] 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/.../.nvm/versions/node/v8.9.1/bin/node] 6: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/.../.nvm/versions/node/v8.9.1/bin/node] 7: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/.../.nvm/versions/node/v8.9.1/bin/node] 8: 0x2fbc95e0463d 9: 0x2fbc95ef4048

Can you file a task to fix this?

cpojer · 2017-11-28T08:23:44Z

packages/jest-runtime/src/index.js

+    const isNativeModule =
+      moduleName &&
+      ((options && options.isNativeModule) ||
+        this._resolver.isCoreModule(moduleName));


woah.. pretty :P

cpojer · 2017-11-28T08:24:38Z

packages/jest-runtime/src/index.js

+    if (isNativeModule) {
+      moduleRegistry = this._nativeModuleRegistry;
+    } else if (options && options.isInternalModule) {
+      moduleRegistry = this._internalModuleRegistry;


The internal module registry is used in Jest for Jest's own core modules like pretty-format or chalk. Any reason you can't reuse that one? Why do you need a third one?

Because of clashes. Once you start requiring inside a Jest internal, all the child modules are required inside Jest are internal modules as well (which is fine, because you prevent clashes between test and Jest code).

If Jest requires internal/util, internal refers to NPM's module; but if you require it from the Native util module, it refers to the internal file; so you need a separate require implementation (as well as a separate registry) to avoid the conflict.

Ok, that makes sense.

cpojer · 2017-11-28T08:27:26Z

packages/jest-runtime/src/index.js

    Object.defineProperty(localModule, 'require', {
      value: this._createRequireImplementation(filename, options),
    });

+    const fileSource = isNativeModule
+      ? // $FlowFixMe: process.binding exists.
+        process.binding('natives')[filename]


is this call fast or slow?

Really fast. The following code:

console.time('binding'); for (let i = 0; i < 100000; i++) { global.fsSourceCode = process.binding('natives').fs; } console.timeEnd('binding');

Reports an average of 16ms per 100,000 calls.

cpojer · 2017-11-28T08:28:16Z

packages/jest-runtime/src/index.js

+    moduleRegistry: ModuleRegistry,
+    from: Path,
+  ) {
+    switch (moduleName) {


cpojer · 2017-11-28T08:30:04Z

packages/jest-runtime/src/index.js

+        // use native resolution. Native resolution differs from standard
+        // resolution on how requires are resolved (e.g. the "util" module
+        // requires "internal/util", which is not the NPM "internal" module,
+        // but something else).


For this comment, how about:

Use a special resolution when requiring node's internal modules. For example the "util" module requires "internal/util" which isn't a physical directory.

It's shorter and conveys the same message.

cpojer

This is great work, I'm so excited for better sandboxing and fewer memory leaks in Jest. Can you work on some of the inline comments that I left?

Before merging this, here are some more high-level questions that would be great to answer:

Can you show results of --logHeapUsage -i before and after this change?
How does this affect performance of Jest?
Can we reduce this to only two module registries (internal and user) vs. three?

SimenB · 2017-11-28T13:56:16Z

packages/jest-runtime/src/script_transformer.js

@@ -33,7 +33,7 @@ export type Options = {|
  collectCoverage: boolean,
  collectCoverageFrom: Array<Glob>,
  collectCoverageOnlyFrom: ?{[key: string]: boolean, __proto__: null},
-  isCoreModule?: boolean,
+  isNativeModule?: boolean,


why change the name? "native module" in my mind is native addons (https://nodejs.org/api/addons.html), not node core modules.

I suppose it's not public API, but still

We already use a isCoreModule in packages/jest-resolve/src/index.js, which does not quite match the meaning of isNativeModule. When I noticed it I decided to rename all the new additions.

mjesun · 2017-11-28T14:35:40Z

Can you show results of --logHeapUsage -i before and after this change?
Yes, heap consumption grows by approx. a 30%. This is expected, since heap measurement is done before we clean the environment; and now all native modules are re-evaluated.
How does this affect performance of Jest?
Time wise, I could see some inconsistent performance regression of around 10%. Second runs with the change take longer (measured on the same machine). Maybe @thymikee could also run against his test suite, so we get other numbers 🙂
Can we reduce this to only two module registries (internal and user) vs. three?
Unfortunately no 😞, Jest's and Node's internal/util mean different things, so we have to address that to avoid key collisions).

cpojer · 2017-11-28T15:07:49Z

Huh, can you try to run again with node --expose-gc? That will run the garbage collector before logging heap usage. A 30% increase in memory usage goes counter to what this diff is aiming to solve.

mjesun · 2017-11-28T15:10:18Z

As explained, it is expected based on where the memory is being measured.

mjesun · 2017-11-28T16:08:47Z

Added a new commit (also rebased over master); modifying where memory is measured. Memory consumption went down from 156 to 143 MB (i.e ~10% less memory 🙂) while measured in Jest repository.

However, the memory still grows within the first tests, up until it stabilizes around 143 MB. Some simple snapshots between tests (running with -i) show that part of the increase comes from objects storing compiled source code (i.e. cacheFS), but there are many others.

codecov-io · 2017-11-28T22:45:21Z

Codecov Report

Merging #4970 into master will increase coverage by 0.01%.
The diff coverage is 61.64%.

@@            Coverage Diff             @@
##           master    #4970      +/-   ##
==========================================
+ Coverage   60.25%   60.27%   +0.01%     
==========================================
  Files         198      198              
  Lines        6623     6659      +36     
  Branches        4        3       -1     
==========================================
+ Hits         3991     4014      +23     
- Misses       2632     2645      +13

Impacted Files	Coverage Δ
packages/jest-runner/src/run_test.js	`1.96% <0%> (-0.27%)`	⬇️
packages/jest-jasmine2/src/index.js	`5.47% <0%> (-0.41%)`	⬇️
packages/jest-runtime/src/script_transformer.js	`86.5% <100%> (ø)`	⬆️
packages/jest-util/src/index.js	`100% <100%> (+11.76%)`	⬆️
packages/jest-util/src/install_common_globals.js	`100% <100%> (ø)`	⬆️
packages/jest-message-util/src/index.js	`86.13% <66.66%> (-0.6%)`	⬇️
packages/jest-runtime/src/index.js	`74.83% <86.04%> (+1.2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0338a82...cf83dff. Read the comment docs.

cpojer · 2017-11-29T08:17:33Z

packages/jest-runner/src/run_test.js

+  };
+
+  let runtime = new Runtime(
+    config,


I think it's time to turn this constructor into something that takes an object.

Should we make this change now? This would break Runtime's interface; that's why I thought it was better to keep it as-is.

No, that should be a follow-up but we could do it for Jest 22.

cpojer · 2017-11-29T08:17:49Z

packages/jest-runner/src/run_test.js

    // Delay the resolution to allow log messages to be output.
    return new Promise(resolve => {
      setImmediate(() => resolve({leakDetector, result}));
    });
  } finally {
-    await environment.teardown();
+    environment.teardown && (await environment.teardown());
+    runtime.teardown && (await runtime.teardown());


No way. Can your turn these into if statements?

cpojer · 2017-11-29T08:18:44Z

packages/jest-runtime/src/index.js

@@ -130,6 +134,8 @@ class Runtime {
    this._mockRegistry = Object.create(null);
    this._moduleMocker = this._environment.moduleMocker;
    this._moduleRegistry = Object.create(null);
+    this._nativeModuleRegistry = Object.create(null);
+    this._path = path || '<unknown test file>';


Can you change this to "unknown entry point"? jest-runtime can be used as standalone and doesn't have to run test files.

cpojer · 2017-11-29T08:19:04Z

packages/jest-runtime/src/index.js

@@ -267,40 +273,68 @@ class Runtime {
    return cliOptions;
  }

+  teardown() {


Can we call this reset and call it in the constructor also for less duplication?

cpojer · 2017-11-29T08:36:06Z

packages/jest-runtime/src/index.js

+    if (!this._environment) {
+      throw new Error(
+        `A module was required after the test suite ${this._path} finished.\n` +
+          `This usually means that you forgot to clean or mock an async operation`,


nit: "In most cases this is because an async operation wasn't cleaned up or mocked properly."

cpojer

Awesome. Please rebase + remove the merged PR + fix some of the nits before merging. I'm still awaiting the memory difference of this on the two large codebases internally, mind sending me that within FB? :)

SimenB · 2017-11-29T13:36:27Z

This should probably have an entry in the changelog

This reverts commit ef55e89.

* Revert "docs: update expect.anything() example (#5007)" This reverts commit ea3fabc. * Revert "Emphasise required return (#4999)" This reverts commit 4f1113c. * Revert "Makes NPM a link like Yarn is (#4998)" This reverts commit aa4f09d. * Revert "Update Timeout error message to `jest.timeout` and display current timeout value (#4990)" This reverts commit 08f8394. * Revert "fix: jest-util should not depend on jest-mock (#4992)" This reverts commit 4e2f41f. * Revert "Update Troubleshooting.md (#4988)" This reverts commit 6414f28. * Revert "Revert "Add the Yammer logo to the 'who is using this' section of the website." (#4987)" This reverts commit 91b104f. * Revert "Make "weak" optional dependency and check it at runtime (#4984)" This reverts commit e00529d. * Revert "Re-inject native Node modules (#4970)" This reverts commit ef55e89.

github-actions · 2021-05-13T02:30:27Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label Nov 27, 2017

mjesun requested review from cpojer and thymikee November 27, 2017 16:09

thymikee reviewed Nov 27, 2017

View reviewed changes

SimenB reviewed Nov 27, 2017

View reviewed changes

thymikee reviewed Nov 27, 2017

View reviewed changes

SimenB reviewed Nov 27, 2017

View reviewed changes

jest-inspect

@@ -0,0 +1,9 @@

#!/usr/bin/env node --inspect-brk

Copy link

Member

SimenB Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fancy :D

mjesun reacted with laugh emoji

SimenB reviewed Nov 27, 2017

View reviewed changes

cpojer reviewed Nov 28, 2017

View reviewed changes

packages/jest-runtime/src/index.js

moduleRegistry: ModuleRegistry,

from: Path,

) {

switch (moduleName) {

Copy link

Member

cpojer Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it.

cpojer reviewed Nov 28, 2017

View reviewed changes

cpojer requested changes Nov 28, 2017

View reviewed changes

SimenB reviewed Nov 28, 2017

View reviewed changes

mjesun mentioned this pull request Nov 28, 2017

Change the way setting up the framework works #4976

Merged

cpojer reviewed Nov 29, 2017

View reviewed changes

cpojer approved these changes Nov 29, 2017

View reviewed changes

Miguel Jimenez Esun and others added 4 commits November 29, 2017 11:27

Re-inject native Node modules

3f1e93d

Fix jest-editor-support test

2d331bc

Addressed feedback

6cfab51

Update index.js

cf83dff

cpojer merged commit ef55e89 into jestjs:master Nov 29, 2017

mjesun deleted the re-inject-native-modules branch December 4, 2017 11:15

mjesun added a commit that referenced this pull request Dec 4, 2017

Revert "Re-inject native Node modules (#4970)"

bb0b4d7

This reverts commit ef55e89.

mjesun mentioned this pull request Dec 4, 2017

Revert "Re-inject native Node modules" #5009

Merged

mjesun added a commit that referenced this pull request Dec 4, 2017

Revert "Re-inject native Node modules (#4970)"

f02c3aa

This reverts commit ef55e89.

mjesun mentioned this pull request Dec 6, 2017

Revert "Add new "setupTestFramework" option (#4976)" #5025

Merged

mjesun mentioned this pull request Dec 22, 2017

process.cwd is not mocked for path.resolve #5146

Closed

mjesun mentioned this pull request Jan 2, 2018

-i has memory issue? #5157

Closed

mjesun mentioned this pull request Aug 13, 2018

Jest leaks memory from required modules with closures over imports #6814

Open

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Re-inject native Node modules #4970

Re-inject native Node modules #4970

Conversation

mjesun commented Nov 27, 2017 • edited Loading

SimenB commented Nov 27, 2017

mjesun commented Nov 27, 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

mjesun Nov 27, 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

SimenB Nov 27, 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

SimenB commented Nov 27, 2017

mjesun commented Nov 27, 2017 • edited Loading

thymikee commented Nov 27, 2017

cpojer commented Nov 28, 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

mjesun Nov 28, 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

mjesun Nov 28, 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

mjesun Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjesun Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjesun commented Nov 28, 2017 • edited Loading

cpojer commented Nov 28, 2017

mjesun commented Nov 28, 2017

mjesun commented Nov 28, 2017 • edited Loading

codecov-io commented Nov 28, 2017 • edited Loading

Codecov Report

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

mjesun commented Nov 27, 2017 •

edited

Loading

mjesun Nov 27, 2017 •

edited

Loading

SimenB Nov 27, 2017 •

edited

Loading

mjesun commented Nov 27, 2017 •

edited

Loading

mjesun Nov 28, 2017 •

edited

Loading

mjesun Nov 28, 2017 •

edited

Loading

mjesun Nov 28, 2017 •

edited

Loading

mjesun Nov 28, 2017 •

edited

Loading

mjesun commented Nov 28, 2017 •

edited

Loading

mjesun commented Nov 28, 2017 •

edited

Loading

codecov-io commented Nov 28, 2017 •

edited

Loading