Improve test coverage #34
Conversation
- Move configuration parsing out of engine and into binstub
- Add .eslintignore from inferred configuration
- Add .dockerignore
- Update .estlintrc to newer defaults
- Small changes for clarity
b2eb553 to
7ebbfab
Compare
| buf.toString().split('\0').forEach(function(json) { | ||
| try { | ||
| issues.push(JSON.parse(json)); | ||
| } catch(e) { } |
There was a problem hiding this comment.
Catching all exceptions when you expect the result to be empty seems counterintuitive (since unexpected exceptions, as opposed to expected exceptions, could indeed cause your result to be empty for reasons other than you were trying to test for). Is the catch needed?
There was a problem hiding this comment.
@wfleming Issues are null terminated, not null separated, so this catch was my sneaky way of dealing with the extra empty string. I think your point is this could hide a bug which I agree with, so I'm just going to trim the last character before parsing. See IssueBuffer.
de03dad to
1b183f0
Compare
d608dfc to
3539eb5
Compare
| @@ -6,8 +6,6 @@ WORKDIR /usr/src/app/ | |||
| COPY engine.json / | |||
| COPY package.json /usr/src/app/ | |||
|
|
|||
| ENV NODE_ENV production | |||
There was a problem hiding this comment.
Hmm, how were our circle builds passing before if devDependencies weren't installed? I guess this env var had no effect?
There was a problem hiding this comment.
We were packaging node_modules up along in the Docker image. I added it to .dockerignore so it stopped doing so.
There was a problem hiding this comment.
Ahh, I see. CircleCI has an inferred npm install step. Can we override that step to be a no-op?
* Move configuration parsing out of engine and into binstub * Add .eslintignore from inferred configuration * Add .dockerignore * Update .estlintrc to newer defaults * Small changes for clarity
3539eb5 to
1457955
Compare
|
LGTM |