coverage: expose native V8 coverage #22527

bcoe · 2018-08-25T23:05:06Z

This pull request is again part of an ongoing initiative to improve the Node.js for those building developer tools (CC: @bengl, @boneskull).

What is this?

For some time now, V8 has exposed test coverage through the inspector protocol. This pull request makes it possible to have this information output to disk by setting the environment variable NODE_V8_COVERAGE=./coverage-directory.

By moving this logic into Node.js itself, we're able to better hook process.exit(), signal-handling, and child_process.spawn(); this is awesome, because this behavior was previously facilitated by a handful of horrible hacks:

spawn-wrap.
signal-exit.

Going forward a tool similar to nyc will simply be able to consume the raw V8 coverage information, and output Istanbul coverage reports.

Help with code review

@nodejs/v8-inspector I rely on some of the behavior that the inspector API is currently exhibiting, mainly that coverage is output in a single tick of the event loop:

    session.post('Profiler.takePreciseCoverage', (err, coverageInfo) => {
      try {
        if (err) throw err;
        writeFileSync(target, JSON.stringify(coverageInfo));
      } catch (err) {
        console.error(err);
      }
    });

It would be wonderful to have some folks double check my work.

I'm also looping in several folks who've been involved with Istanbul and nyc over the years.

@isaacs, @addaleax, @schuay.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-25T23:05:07Z

@bcoe build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/674/pipeline

devsnek · 2018-08-25T23:22:35Z

lib/child_process.js

@@ -496,6 +496,12 @@ function normalizeSpawnArguments(file, args, options) {
  var env = options.env || process.env;
  var envPairs = [];

+  // process.env.NODE_V8_COVERAGE always propagates, making it possible to
+  // collect coverage for programs that spawn with white-listed environment.
+  if (process.env.NODE_V8_COVERAGE) {


is there a way to stop this from happening?

there is not at this time? any recommendations.

It's almost always the case that you will be setting NODE_V8_COVERAGE in the context of running a test suite, and that you'd probably like to er on the side of the variable propagating, even though it's pretty common in tests to selectively set env, e.g., maybe your test is exercising a specific environment.

I'm sure it's quite common, but we should support both sides. perhaps checking if the property already exists (so you can set it to an empty string) would work.

I think that's a smart idea, I was thinking even for our own test suite we might want to be able to remove the variable.

devsnek · 2018-08-25T23:26:14Z

doc/api/cli.md

@@ -625,6 +625,15 @@ Path to the file used to store the persistent REPL history. The default path is
 `~/.node_repl_history`, which is overridden by this variable. Setting the value
 to an empty string (`''` or `' '`) disables persistent REPL history.

+### `NODE_V8_COVERAGE=dir`
+
+When set, Node.js will begin outputting [V8 JavaScript code coverage][] to the


neither the link nor the addition here explain what to do with the output coverage file. Generally people just use the inspector UI. does the output file even have a governed format? do we have to semver major when v8 changes the format?

Generally people just use the inspector UI

I would argue it's very rare for people writing command line tools and modules to interact with the inspector, as evidenced by the fact that nyc has over 31,000 dependents and continues to grow in popularity (getting downloaded over 400,000 times a week) despite native test coverage having been available for quite some time.

As the lead maintainer of nyc and Istanbul, I would love to move people away from using these libraries when writing Node.js applications, and towards V8's coverage -- but I think the burden of kicking off and interacting with an inspector session is a usability rough-edge that will prevent adoption.

do we have to semver major when v8 changes the format?

I think you're right that we should point users to a breakdown of what to expect in the output format -- @schuay can you recommend a better document to link to.

The output format is standardized in V8, and I don't expect that there will be major breaking changes to it (and if there are it will be very rare).

sorry I wasn't clear. I'm saying there need to be docs for the format available from the node docs, not that people should only use the inspector UI. the link you have doesn't give any docs on the format of the file.

On the semver-major front, I just want to make sure we're careful, if you think it's fine that's good enough for me.

let's let @schuay or someone else on V8's side chime in too (@TimothyGu?), I think your concern is valid and it would be good to have someone speak to how stable the API is on the Chromium side of things.

Sorry, I'm not too involved in the V8 development process… though it might be telling that the coverage API is already in the RC version of the devtools protocol without an Experimental flag on.

Great to see this 👍

The inspector protocol documentation here is the source of truth. In particular, takePreciseCoverage will return an array of ScriptCoverage objects.

I'm not sure what DevTools' policies on protocol changes are. You could try asking on their mailing list: https://groups.google.com/forum/#!forum/google-chrome-developer-tools. For V8 coverage specifically, we currently don't have any changes in the output format planned that I'm aware of.

Generally we are very careful with changing CDP due to backwards compatibility concerns. Unless unavoidable, we will not change the coverage format. If that happens however, the CDP version would be bumped as well.

addaleax · 2018-08-26T00:13:38Z

lib/internal/process/coverage.js

+    return;
+  }
+
+  const filename = `coverage-${process.pid}-${Date.now()}.json`;


I think we need to account for the worker thread id here, and test interaction with it, as well…

mind expanding a bit, pseudo code would be awesome 👍

Basically, I’m asking for require('internal/worker').threadId to be part of the format string here :)

addaleax · 2018-08-26T00:16:13Z

test/parallel/test-v8-coverage.js

+  const output = spawnSync(process.execPath, [
+    require.resolve('../fixtures/v8-coverage/exit-1')
+  ], { env: { NODE_V8_COVERAGE: coverageDirectory } });
+  assert.strictEqual(output.status, 1, 'exit code not 1');


We generally try to avoid using static message parameters for assertions, and use comments instead – most of the time, a static message string just hides the most useful information (the actual values in the assertion)

vsemozhetbyt · 2018-08-26T08:11:25Z

doc/api/cli.md

@@ -695,3 +704,4 @@ greater than `4` (its current default value). For more information, see the
 [emit_warning]: process.html#process_process_emitwarning_warning_type_code_ctor
 [experimental ECMAScript Module]: esm.html#esm_loader_hooks
 [libuv threadpool documentation]: http://docs.libuv.org/en/latest/threadpool.html
+[V8 JavaScript code coverage]: https://v8project.blogspot.com/2017/12/javascript-code-coverage.html


Nit: this reference should go after [REPL]: repl.html in ASCII sort order.

vsemozhetbyt · 2018-08-26T08:14:28Z

doc/api/cli.md

+directory provided as an argument.
+
+`NODE_V8_COVERAGE` will automatically propagate to subprocesses, making it
+easier to instrument applications that call the `child_process.spawn` family


Nit: `child_process.spawn` -> `child_process.spawn()`

devsnek · 2018-08-28T01:30:48Z

lib/child_process.js

+  // process.env.NODE_V8_COVERAGE always propagates, making it possible to
+  // collect coverage for programs that spawn with white-listed environment.
+  if (process.env.NODE_V8_COVERAGE &&
+      !(options.env || {}).hasOwnProperty('NODE_V8_COVERAGE')) {


please use Object.prototype.hasOwnProperty.call

changes

addaleax · 2018-08-28T23:09:34Z

doc/api/cli.md

+
+When set, Node.js will begin outputting [V8 JavaScript code coverage][] to the
+directory provided as an argument. Coverage is output as an array of
+[ScriptCoverage][] objects.


Just wondering, would an object with a single entry make sense? That would leaves a bit of room for adding more information, e.g. metadata?

the object is shaped like this:

{ "results": [ScriptCoverage, ScriptCoverage] }

Okay, great – that just wasn’t obvious from the documentation :)

bcoe · 2018-08-29T00:43:32Z

@nodejs/testing 👋 thought this would be pertinent to your interests -- a next logical step once this lands would be moving Node.js' test coverage over to the approach. I've benchmarked it and it's 300% faster 😮

evanlucas · 2018-08-29T02:28:31Z

I’m super thrilled to see this! Awesome work!

evanlucas · 2018-08-29T02:33:56Z

lib/internal/process/coverage.js

+
+  const filename = `coverage-${process.pid}-${Date.now()}.json`;
+  try {
+    mkdirSync(process.env.NODE_V8_COVERAGE);


Should this use the recursive option?

I'd like to switch to recursive as soon as this issue is resolved, and we know the shape of the API won't be changing.

This is actually a perfect example of why mkdirp is a good addition to the API :)

Trott · 2018-08-31T03:52:46Z

I suspect some members of @nodejs/build may be familiar with coverage stuff. Ping!

BridgeAR · 2018-08-31T05:03:59Z

lib/child_process.js

+  // process.env.NODE_V8_COVERAGE always propagates, making it possible to
+  // collect coverage for programs that spawn with white-listed environment.
+  if (process.env.NODE_V8_COVERAGE &&
+ !Object.prototype.hasOwnProperty.call(options.env || {}, 'NODE_V8_COVERAGE')) {


No-blocking nit: !options.env || !Object.prototype.hasOwnProperty.call(options.env, 'NODE_V8_COVERAGE')

BridgeAR · 2018-08-31T05:05:38Z

doc/api/cli.md

+
+`NODE_V8_COVERAGE` will automatically propagate to subprocesses, making it
+easier to instrument applications that call the `child_process.spawn()` family
+of functions.


Adding a note that this is currently not supported for worker threads is likely a good thing as people might otherwise maybe run into this as long as it's not supported.

It should also be documented that the coverage generation can be deactivated for specific child processes by explicitly setting the env flag to an empty string.

BridgeAR · 2018-08-31T05:11:11Z

lib/internal/process/coverage.js

+  try {
+    session.post('Profiler.takePreciseCoverage', (err, coverageInfo) => {
+      try {
+        if (err) throw err;


Moving this one line up would be better:

if (err) return console.error(err); try { ... } catch (err) { ... }

BridgeAR · 2018-08-31T05:20:26Z

test/parallel/test-v8-coverage.js

+function getFixtureCoverage(fixtureFile, coverageDirectory) {
+  const coverageFiles = fs.readdirSync(coverageDirectory);
+  for (let i = 0, coverageFile; (coverageFile = coverageFiles[i]) !== undefined;
+    i++) {


This looks like it should be written with for of loops instead:

for (const coverageFile of coverageFiles) { ... for (const fixtureCoverage of coverage.result) { ... } }

BridgeAR · 2018-08-31T05:22:27Z

test/parallel/test-v8-coverage.js

+  const coverageDirectory = path.join(tmpdir.path, nextdir());
+  const output = spawnSync(process.execPath, [
+    require.resolve('../fixtures/v8-coverage/basic')
+  ], { env: extendEnv({ NODE_V8_COVERAGE: coverageDirectory }) });


I suggest to use the object spread notation instead of using a helper function here.

{ env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }

BridgeAR · 2018-08-31T05:24:13Z

lib/internal/process/coverage.js

+
+function writeCoverage() {
+  if (!hasInspector || !session) {
+    return;


The second part of the condition is not possible to reach: either there is no inspector in which case the session is empty or the session will be set (if that would not be the case, the property access below would throw).

And should we not log an error here? I would be surprised if I pass in the environment variable but the coverage files are not generated.

BridgeAR · 2018-08-31T05:28:03Z

lib/internal/process/coverage.js

+
+function setup() {
+  if (!hasInspector) {
+    return;


I suggest to throw in error in this case. Otherwise the user would not know that no data will be collected.

In that case the hasInspector check in writeCoverage would also be obsolete.

I’m not a fan of throwing here. If necessary, we can print a warning, but in practice, this situation really only occurs within Workers (for now).

I've added a warning, which I feel is a reasonable compromise; It would be miserable to be unable to run your test suite with coverage because executing code in a worker results in exceptions being thrown.

When we support workers in the future, perhaps we could switch to throwing an exception?

rvagg · 2018-08-31T05:47:01Z

lib/child_process.js

+  // process.env.NODE_V8_COVERAGE always propagates, making it possible to
+  // collect coverage for programs that spawn with white-listed environment.
+  if (process.env.NODE_V8_COVERAGE &&
+ !Object.prototype.hasOwnProperty.call(options.env || {}, 'NODE_V8_COVERAGE')) {


bad indentation

rvagg · 2018-08-31T06:00:16Z

I imagine this will let us replace lib/internal/process/write-coverage.js functionality, does that have to be a separate step?

hashseed · 2018-08-31T07:42:29Z

lib/internal/process/coverage.js

+  session.connect();
+  session.post('Profiler.enable');
+  session.post('Profiler.startPreciseCoverage', { callCount: true,
+                                                  detailed: true });


Do we want to also expose a way to use other coverage modes? This primarily has performance implications. E.g. callCount: false would allow a function to be inlined after it has been covered. detailed: true would not require coverage-related bytecode to be emitted.

I'd argue for simplicity, exposing only the most detailed mode, and making sure it is as fast as possible. callCount: true / detailed: true should also allow inlining (it does require special bytecode emission though).

I'm with @schuay on this one, I'd lean towards starting simple. I can imagine following up with the option for more complex configuration.

addaleax · 2018-09-02T12:48:51Z

@bcoe Heads up, this needs a rebase (presumably around the options help texts being moved to lib/internal/print_help.js)

bcoe · 2018-09-03T00:56:07Z

@rvagg I'd like to replace write-coverage.js and the instrumentation process we apply with this approach eventually (it will most likely be the thing I work on immediately after we land this, with @addaleax's help).

I'd prefer to spread this out over two pull requests, for a few reasons:

a few tests will need to be debugged and potentially rewritten a bit; specifically tests that exercise async_hooks are having trouble passing in Node's suite, because they mock some of the internal libraries used by the inspector.
all the coverage bits in the Makefile will need to be rewritten and fiddled with.
I need to actually write the dependent library that easily converts from the inspector format to the format consumed by our reports (I have most of the pieces written, but it will be easier to finish off this dependency once we land this work).

tldr; I think there are enough sticky bits to getting Node consuming the inspector coverage format, that it will be easier to consume across two pull requests.

BridgeAR

LGTM with my last two comments addressed.

BridgeAR · 2018-09-04T07:24:51Z

lib/internal/process/coverage.js

@@ -28,8 +28,8 @@ function writeCoverage() {

  try {
    session.post('Profiler.takePreciseCoverage', (err, coverageInfo) => {
+      if (err) console.error(err);


If an error exists, the code should not continue, otherwise the write would still be triggered.

Adding a return statement fixes this: if (err) return console.error(err);

BridgeAR · 2018-09-04T07:32:58Z

lib/internal/process/coverage.js

@@ -9,7 +9,7 @@ const hasInspector = process.config.variables.v8_enable_inspector === 1 &&
 let session;

 function writeCoverage() {
-  if (!hasInspector || !session) {
+  if (!session) {
    return;


The session check can not be reached due to either being set to a truthy value if the inspector is present, or not being set and the setup function throws and the code does not continue. If the latter is the case, the writeCoverage function is never called at all.

~~Therefore it's safe to remove this check as well.~~

I missed that the function is exported.

jasnell · 2018-09-04T16:32:19Z

test/parallel/test-v8-coverage.js

@@ -20,6 +20,18 @@ function nextdir() {
  const output = spawnSync(process.execPath, [
    require.resolve('../fixtures/v8-coverage/basic')
  ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } });
+  try {
+    console.info('--- DEBUG ---');


are the console.info() statements part of the test or purely informational? If informational, I'd prefer to omit them and use code comments instead. If they are part of the test, then please add a comment indicating that so that they are not accidentally removed later.

there's a new build environment that fails for coverage, added since I last ran the suite. This is purely to see what the heck is going on:

node-test-commit-linux-containered

will remove as soon as I see what the underlying exception is.

native V8 coverage reports can now be written to disk by setting the variable NODE_V8_COVERAGE=dir

bcoe · 2018-09-05T05:32:15Z

I believe comments have been addressed, and I intend to land this in the next day or so 👍

bcoe · 2018-09-05T05:48:14Z

ci: https://ci.nodejs.org/job/node-test-pull-request/17021/ ✔️

native V8 coverage reports can now be written to disk by setting the variable NODE_V8_COVERAGE=dir PR-URL: #22527 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rod Vagg <rod@vagg.org>

bcoe · 2018-09-05T17:30:49Z

Landed in c9d6e3f

native V8 coverage reports can now be written to disk by setting the variable NODE_V8_COVERAGE=dir PR-URL: #22527 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22394 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22392 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716

native V8 coverage reports can now be written to disk by setting the variable NODE_V8_COVERAGE=dir PR-URL: #22527 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Rod Vagg <rod@vagg.org>

Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22407 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22515 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716

bcoe added semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. inspector Issues and PRs related to the V8 inspector protocol labels Aug 25, 2018

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 25, 2018

bcoe requested review from addaleax, Trott and MylesBorins August 25, 2018 23:05

bcoe force-pushed the coverage branch from 97a6c65 to 03a7833 Compare August 25, 2018 23:11

devsnek previously requested changes Aug 25, 2018

View reviewed changes

addaleax reviewed Aug 26, 2018

View reviewed changes

vsemozhetbyt reviewed Aug 26, 2018

View reviewed changes

vmarchaud mentioned this pull request Aug 26, 2018

expose gc event as inspector's runtime event nodejs/diagnostics#220

Closed

bcoe force-pushed the coverage branch from 193ab84 to fc3e539 Compare August 27, 2018 22:14

devsnek reviewed Aug 28, 2018

View reviewed changes

addaleax approved these changes Aug 28, 2018

View reviewed changes

evanlucas reviewed Aug 29, 2018

View reviewed changes

BridgeAR reviewed Aug 31, 2018

View reviewed changes

rvagg reviewed Aug 31, 2018

View reviewed changes

hashseed reviewed Aug 31, 2018

View reviewed changes

hashseed approved these changes Aug 31, 2018

View reviewed changes

bcoe force-pushed the coverage branch from d128c9e to 5370fec Compare September 3, 2018 04:49

BridgeAR approved these changes Sep 4, 2018

View reviewed changes

evanlucas approved these changes Sep 4, 2018

View reviewed changes

jasnell reviewed Sep 4, 2018

View reviewed changes

coverage: expose native V8 coverage

0ec2a60

native V8 coverage reports can now be written to disk by setting the variable NODE_V8_COVERAGE=dir

bcoe force-pushed the coverage branch from c5188e1 to 0ec2a60 Compare September 5, 2018 00:40

rvagg approved these changes Sep 5, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018

bcoe changed the title ~~coverage: expose native V8 coverage~~ test: expose native V8 coverage Sep 5, 2018

bcoe changed the title ~~test: expose native V8 coverage~~ coverage: expose native V8 coverage Sep 5, 2018

bcoe closed this Sep 5, 2018

bcoe deleted the coverage branch September 5, 2018 18:30

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

bcoe mentioned this pull request Sep 8, 2018

feat: switch to using Node's built in coverage bcoe/c8#22

Merged

addaleax added the coverage Issues and PRs related to native coverage support. label Sep 18, 2018

OmgImAlexis mentioned this pull request Sep 20, 2018

todo: figure out how to instrument .mjs files istanbuljs/nyc#659

Open

kt3k mentioned this pull request Jan 20, 2019

feature-request: first-class test-coverage support denoland/deno#106

Closed

coverage: expose native V8 coverage #22527

coverage: expose native V8 coverage #22527

Conversation

bcoe commented Aug 25, 2018

What is this?

Help with code review

Checklist

nodejs-github-bot commented Aug 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe Aug 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe Aug 25, 2018 • 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

schuay Aug 27, 2018 • 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

addaleax Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

bcoe Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Aug 29, 2018

evanlucas commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 31, 2018 • 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

rvagg commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Sep 2, 2018

bcoe commented Sep 3, 2018 • edited Loading

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

bcoe commented Sep 5, 2018

bcoe commented Sep 5, 2018 • edited by BridgeAR Loading

bcoe commented Sep 5, 2018

bcoe Aug 26, 2018 •

edited

Loading

bcoe Aug 25, 2018 •

edited

Loading

schuay Aug 27, 2018 •

edited

Loading

addaleax Aug 28, 2018 •

edited

Loading

bcoe Aug 28, 2018 •

edited

Loading

Trott commented Aug 31, 2018 •

edited

Loading

bcoe commented Sep 3, 2018 •

edited

Loading

BridgeAR Sep 4, 2018 •

edited

Loading

bcoe Sep 4, 2018 •

edited

Loading

bcoe commented Sep 5, 2018 •

edited by BridgeAR

Loading