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

add `sourcecred/scores` #1223

Merged
merged 2 commits into from Jul 14, 2019

Conversation

@decentralion
Copy link
Member

commented Jul 12, 2019

The scores are lightly processed from their internal representation.
Example usage:

$ yarn backend;
$ node bin/sourcecred.js load sourcecred/sourcecred
$ node bin/sourcecred.js scores sourcecred/sourcecred > scores.json

The data structure is as follows:

export type NodeOutput = {|
  +id: string,
  +totalCred: number,
  +intervalCred: $ReadOnlyArray<number>,
|};

export type ScoreOutput = Compatible<{|
  +users: $ReadOnlyArray<NodeOutput>,
  +intervals: $ReadOnlyArray<Interval>,
|}>;

Test plan: I added sharness tests at sharness/test_cli_scores.t.
In the past, we've used javascript tests for CLI commands. However,
those are pretty time-consuming to write, and are less robust than
simply running the command from bash. Check the snapshot for a sense of
what the new data format looks like. Also, the snapshot updater now
updates this snapshot too.

Relevant for #1047.
Thanks to @Beanow for feedback on the output format and design.
Thanks to @wchargin for help in code review.

@@ -7,6 +7,7 @@ import dedent from "../util/dedent";
import {help as loadHelp} from "./load";
import {help as analyzeHelp} from "./analyze";
import {help as pagerankHelp} from "./pagerank";

This comment has been minimized.

Copy link
@Beanow

Beanow Jul 12, 2019

Member

pagerankHelp is somehow missing below and shows in the linter as unused import.

This comment has been minimized.

Copy link
@decentralion

decentralion Jul 12, 2019

Author Member

Fixed.

@Beanow

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

This looks like a very usable output format. Should we include a format version for upgrade sanity sake?

@decentralion decentralion force-pushed the cli-scores branch from 65a8016 to 9a7032d Jul 12, 2019

@decentralion

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

@Beanow I added versioning info per your suggestion. I also added sharness tests with a snapshot, so you can see what the output format looks like.

@decentralion decentralion force-pushed the cli-scores branch from 9a7032d to 1189a93 Jul 12, 2019

add `sourcecred/scores`
The scores are lightly processed from their internal representation.
Example usage:

```
$ yarn backend;
$ node bin/sourcecred.js load sourcecred/sourcecred
$ node bin/sourcecred.js scores sourcecred/sourcecred > scores.json
```

The data structure is as follows:

```js
export type NodeOutput = {|
  +id: string,
  +totalCred: number,
  +intervalCred: $ReadOnlyArray<number>,
|};

export type ScoreOutput = Compatible<{|
  +users: $ReadOnlyArray<NodeOutput>,
  +intervals: $ReadOnlyArray<Interval>,
|}>;
```

Test plan: I added sharness tests at `sharness/test_cli_scores.t`.
In the past, we've used javascript tests for CLI commands. However,
those are pretty time-consuming to write, and are less robust than
simply running the command from bash. Check the snapshot for a sense of
what the new data format looks like. Also, the snapshot updater now
updates this snapshot too.

@decentralion decentralion force-pushed the cli-scores branch from 1189a93 to b32b882 Jul 12, 2019

run() (
set -eu
rm -f out err
node "${SOURCECRED_BIN}"/sourcecred.js "$@" >out 2>err

This comment has been minimized.

Copy link
@decentralion

decentralion Jul 12, 2019

Author Member

@wchargin would you be willing to take a look at this file and tell me what you think?

This comment has been minimized.

Copy link
@wchargin
@Beanow

Beanow approved these changes Jul 12, 2019

Copy link
Member

left a comment

The header looks good to me. Assuming the test issue is resolved I'm happy with this.

test_expect_success "environment and Node linking setup" '
toplevel="$(git -C "$(dirname "$0")" rev-parse --show-toplevel)" &&
snapshot_directory="${toplevel}/sharness/__snapshots__/" &&
SOURCECRED_DIRECTORY="${snapshot_directory}/example-github-load" &&

This comment has been minimized.

Copy link
@wchargin

wchargin Jul 13, 2019

Member

You haven’t exported SOURCECRED_DIRECTORY, so it’s not visible to the
child processes. The children work against your actually-populated cache
locally, but fail with an empty cache on CI (or on my machine,
presumably, though I didn’t run this).

Immediate fix:

    SOURCECRED_DIRECTORY="${snapshot_directory}/example-github-load" &&
    export SOURCECRED_DIRECTORY &&

Long-term suggestion: In the top-level test config, make a temp
directory and export `${tmpdir}/ERROR_NO_SOURCECRED_DIRECTORY_SET`
to make tests more hermetic.

Also:

  • In your test, use diff -u rather than diff -q so that the output
    is printed on failure.

  • In your run, use something like

    run() (
      set -eu
      rm -f out err
      code=0
      node "${SOURCECRED_BIN}"/sourcecred.js "$@" >out 2>err || code=$?
      if [ "${code}" -ne 0 ]; then
        printf '%s failed with %d\n' "sourcecred $*"
        printf 'stdout:\n'
        cat out
        printf 'stderr:\n'
        cat err
      fi
      return "${code}"
    )

    so that you can see the output when it fails.

This comment has been minimized.

Copy link
@Beanow

Beanow Jul 13, 2019

Member

Related to #1226

This comment has been minimized.

Copy link
@decentralion

decentralion Jul 14, 2019

Author Member

Thanks for the suggestions! I adopted the all of them except the longer-term suggestion, which I didn't understand. Can you please elaborate on it? Are there any examples of a similar pattern already in the codebase?

Also, why should we separate defining SOURCECRED_DIRECTORY and exporting it into two separate statements? Couldn't we just as easily combine them into one?

This comment has been minimized.

Copy link
@wchargin

wchargin Jul 15, 2019

Member

I adopted the all of them except the longer-term suggestion, which I
didn't understand. Can you please elaborate on it?

Sure.

When you run config/test.js, we invoke (e.g.) yarn run -s sharness
in a subprocess. That subprocess inherits your working environment,
including your SOURCECRED_DIRECTORY variable. If the Sharness task
doesn’t explicitly set the SOURCECRED_DIRECTORY variable itself, then
it will use your global cache, which is not hermetic: it may pass for
you but fail for everyone else, as happened here.

The simplest form of what I’m suggesting is to just add

process.env["SOURCECRED_DIRECTORY"] = "/ERROR_SOURCECRED_DIRECTORY_NOT_SET";

to the top of config/test.js. Then, if the Sharness task fails to
explicitly set the SOURCECRED_DIRECTORY variable, it will inherit the
“dummy” value; any attempt to read from or write to this path, which is
a non-existent directory in a write-protected directory, will be much
more likely to fail.

In the case of the bug in the original version of this PR, I think that
the test would have just failed with

fatal: repository ID sourcecred/example-github not loaded
Try running `sourcecred load sourcecred/example-github` first.

when run locally, which is probably enough to point you in the right
direction. If the test had tried to run sourcecred load …, then the error message would have been

Contents of stderr:
    EACCES: permission denied, mkdir '/ERROR_SOURCECRED_DIRECTORY_NOT_SET'

which explicitly points to the problem in the test configuration.

(I say “simplest” because ideally we’d (a) change yarn sharness
similarly, to prevent skew between yarn sharness and yarn test, and
(b) use a directory name that is guaranteed to not exist by running
something like tmpdir="$(mktemp -d)" && chmod 000 "${tmpdir}" before
exporting "${tmpdir}/ERROR_SOURCECRED_DIRECTORY_NOT_SET".)

Are there any examples of a similar pattern already in the codebase?

Not exactly, no. But the general pattern is occasionally quite useful.

Also, why should we separate defining SOURCECRED_DIRECTORY and
exporting it into two separate statements? Couldn't we just as easily
combine them into one?

Using export NAME=word is in general dangerous when word is not a
constant, because export returns success if the export itself succeeds
even if the expansion of word failed. For instance:

$ (set -e; FOO="$(false)"; printf 'got here\n'; export FOO); echo $?
1
$ (set -e; export FOO="$(false)"; printf 'got here\n'); echo $?
got here
0

It’s actually safe in this case, because the only non-constant portion
of the variable assignment is a parameter expansion, and failures of
parameter expansion (either unbound variables with set -u, or syntax
errors in the expansion form) will cause the whole export to fail.
But this isn’t something that I really want to have to remember, hence
the blanket advice to separate exports from assignments whenever the
RHS of the assignment is non-constant.

That is: if you made this particular export also assign, I wouldn’t
object; the above habit is simply the reason that I wrote my suggestion
the way that I did.

There is a shellcheck lint for this:
https://github.com/koalaman/shellcheck/wiki/SC2155

(and the shellcheck lint will only warn in the actually-dangerous
case, which is nice). I highly recommend integrating shellcheck-on-save
into your editor. It’ll catch all of those missing quotes!

This comment has been minimized.

Copy link
@decentralion

decentralion Jul 15, 2019

Author Member

Thanks!

sharness/test_cli_scores.t Outdated Show resolved Hide resolved

test_expect_success SETUP, \
"should print help message when called without scores" '
test_must_fail run scores &&

This comment has been minimized.

Copy link
@wchargin

wchargin Jul 13, 2019

Member

nit: Please fix indentation throughout this file. It’s 4-space indent,
but more importantly the elements of an &&-chain should all be at the
same level of indentation.

This comment has been minimized.

Copy link
@decentralion

decentralion Jul 14, 2019

Author Member

Done.

sharness/test_cli_scores.t Outdated Show resolved Hide resolved
sharness/test_cli_scores.t Outdated Show resolved Hide resolved
test_expect_success SETUP \
"should be identical to the snapshot" '
run scores sourcecred/example-github &&
diff -q out ${snapshot_file}

This comment has been minimized.

Copy link
@wchargin

wchargin Jul 13, 2019

Member

Quotes!

run() (
set -eu
rm -f out err
node "${SOURCECRED_BIN}"/sourcecred.js "$@" >out 2>err

This comment has been minimized.

Copy link
@wchargin

@decentralion decentralion merged commit 88f736d into master Jul 14, 2019

1 check passed

ci/circleci: test Your tests passed on CircleCI!
Details

@decentralion decentralion deleted the cli-scores branch Jul 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.