Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/browser result parser #38

Merged
merged 20 commits into from
Nov 1, 2017
Merged

Conversation

lsm
Copy link
Contributor

@lsm lsm commented Oct 27, 2017

This PR is for #37. Changes including:

  • Add browser result parser (karma).
  • Add prettier conf for consistent editor behavior (copied from futil).

@decrapifier
Copy link

decrapifier commented Oct 27, 2017

Warnings
⚠️

❗ This PR is BIG (+1050 -516)
Please keep it below 500 net changes

⚠️

You should add at least 1 more reviewer to the PR

⚠️

The README has not been updated. Please update the README.

Messages
📖

Could not find any browser results.

📖

We were able to automatically fix some formatting issues in this PR for you!

Generated by 🚫 dangerJS

@lsm lsm requested a review from sean-clayton October 27, 2017 19:31
Copy link
Contributor

@sean-clayton sean-clayton left a comment

Choose a reason for hiding this comment

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

https://github.com/smartprocure/duti/blob/feature/browser-result-parser/package.json#L21

Can you change that since there's now configuration in package.json for this?


if (result && summary) {
let { error, failed, exitCode } = summary
if (false === error && 0 === failed && 0 === exitCode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

swap these logic tests around please

eg error === false, failed === 0, etc

if (result && summary) {
let { error, failed, exitCode } = summary
if (false === error && 0 === failed && 0 === exitCode) {
message('Your PR has no browser errors. Great job!')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please just use one space after periods

}, failedTests),
// Filter by failed tests.
_.filter(testResult => {
return false === testResult.success
Copy link
Contributor

Choose a reason for hiding this comment

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

Swap logic test here, too

_.mapValues(r => r),
// Flatten test results into one array.
_.reduce((flattened, other) => {
return flattened.concat(other)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do (flattened, other) => flattened.concat(other), or even better, (flattened, other) => [...flattened, other]

}

if (failed > 0) {
let failedTests = []
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason why it's starts as an empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what wanted to know as well. Better solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand _.reduce requires the accumulator as the initial value. I do agree with you, it definitely could be improved.

Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to inline it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just did.

Copy link
Contributor Author

@lsm lsm Oct 27, 2017

Choose a reason for hiding this comment

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

Looks way better now. It would be perfect if we don't have to have that at all ;)

}

// All other cases: error is not false, exitCode is not 0 etc.
fail('Browser test error. Please see CI logs for more details.')
Copy link
Contributor

Choose a reason for hiding this comment

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

One too many spaces after period

fail('Browser test error. Please see CI logs for more details.')
} else {
message(
'Incorrect browser result format. Please see CI logs for more details.'
Copy link
Contributor

Choose a reason for hiding this comment

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

One too many spaces after period

@lsm lsm self-assigned this Oct 27, 2017
_.flow(
// Flatten test results into one array.
_.reduce((flattened, other) => {
return flattened.concat(other)
Copy link
Contributor

Choose a reason for hiding this comment

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

just return directly after the arrow function. You should probably also concat using spreads eg:

(flattened, other) => [...flattened, other]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried this _.reduce((flattened, other) => [...flattened, other], []), but doesn't work. I should probably change the spec to match the output text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, what you want me to do with the package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"lint": "eslint . --ignore-path .gitignore", ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, that's fine then. _.reduce((flattened, other) => flattened.concat(other), []) should do the trick. Just trying to avoid the unnecessary return there 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

also, in package.json, I inlined the options --singleQuote etc in the fmt script, but since we're breaking configuration out, we should remove it in the fmt script

Copy link
Contributor

Choose a reason for hiding this comment

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

"fmt": "prettier --write {*.js,{src,bin}/**/*.js} --no-semi --single-quote --trailing-comma es5"

turns to

"fmt": "prettier --write {*.js,{src,bin}/**/*.js}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks way better.

@sean-clayton
Copy link
Contributor

🎉

@sean-clayton
Copy link
Contributor

@daedalus28 look good?

package.json Outdated
@@ -4,8 +4,12 @@
"bin": {
"duti": "bin/duti"
},
"version": "0.7.1",
"version": "0.7.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably be 0.8.0 because it is a new feature :)

if (failed > 0) {
_.flow(
// Flatten test results into one array.
_.reduce((flattened, other) => flattened.concat(other), []),
Copy link
Contributor

@daedalus28 daedalus28 Oct 30, 2017

Choose a reason for hiding this comment

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

since result in an object, you can just do a _.values and then a _.flatten in the flow

let { execSync } = require('child_process')
let { Repository } = Git
let { getRunningDirectory } = require('../utils')
let _ = require("lodash/fp");
Copy link
Contributor

Choose a reason for hiding this comment

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

why are there double quotes and semicolons here? looks like the prettier config messed up

@daedalus28 daedalus28 merged commit 4961157 into master Nov 1, 2017
@daedalus28 daedalus28 deleted the feature/browser-result-parser branch November 1, 2017 19:45
@ghost ghost removed the A: Marc Liu label Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants