Commit Graph

799 Commits

Author SHA1 Message Date
Dandelion Mané 2a5c093286
Update CHANGELOG.md (#820)
It now mentions that we added `MentionsAuthor` edges to the GitHub
graph in #808.

Thanks @whyrusleeping for suggesting this heuristic.

Test plan: n/a
2018-09-12 20:30:35 -07:00
Dandelion Mané 7d0d4fb2fa
Add GitHub commit entity (#819)
This adds a `Commit` entity to the GitHub relational view. It has all
the standard methods: commits can be retrieved en masse or by particular
address, they have a URL and authors, and (de)serialize appropriately.

The code for adding pull requests has been modified so that the merge
commits are added as commit entities. This does not have any effect on
the ultimate graph being created; the same edge is added either way.

Test plan: I've extended the standard RelationalView tests to cover the
`Commit` entity. The case where the commit has 0 authors is not yet
tested, but will be once I add support for getting all of the commits
from the example-github (we have one example of a commit that doesn't
map to a user).

Progress on #815.
2018-09-12 19:44:26 -07:00
Dandelion Mané 3e06c054db
Give GitHub plugin support for Commit addresses (#816)
The Git plugin owns Commits, but the GitHub plugin also creates commits.
This commit reifies that relationship by making a Git commit address a
valid GitHub structured address. This is precursor work for #815, which
will require adding a commit entity to the GitHub relational view.

Also, this commit surfaces and fixes a minor type bug, wherein a map
from strings to referent addresses was typed to hold any structured
address, rather than just referent addresses.

Test plan: The unit tests confirm that serializing/deserializing a Git
commit address using the GitHub plugin's methods works as intended.
Also, unit tests were added that verify that (de)serializing Git
addresses for non-commit objects is still an error.
2018-09-12 19:06:06 -07:00
Dandelion Mané 7dc9449fe7
GitHub: Request commit authorship info (#817)
This commit pulls the graphql fields to request commit information into
a fragment, and requests GitHub authorship information (when
available) for that fragment. We don't use that information yet, but we
will soon. Progress on #815.

Test plan: Observe that the example-github data is updated, so that we
now have urls and authorship for commits. Observe that the query has
updated, but no downstream code was affected. `yarn test --full` passes.
2018-09-12 18:51:06 -07:00
Dandelion Mané 335441e671
Expose node/edge prefixes publicly (#814)
Both the GitHub and Git plugins create a `_Prefix` object for nodes and
edges, which gives the respective prefixes for different node/edge
types. We named it `_Prefix` because we weren't sure if these should be
exported. In practice, these have proven quite useful to make generally
available, and despite the `_`-naming we expose the objects outside
their modules. This change renames `_Prefix` to `Prefix` to reflect the
reality that these are used as public consts.

Exporting them is safe as both objects are frozen.

Test plan: Simple rename, `yarn test` suffices.
2018-09-12 17:18:47 -07:00
Dandelion Mané 70fe677990
Update example GitHub data (#813)
Generated via `src/plugins/github/fetchGithubRepoTest.sh -u`

`yarn test --full` passes.

Closes #389. Thanks, @wchargin!
2018-09-12 17:18:37 -07:00
Dandelion Mané 737ed4d8b3
Add `MentionsAuthor` edges to the graph (#808)
This commit builds on the work in #806, adding the
`MentionsAuthorReference`s to the graph. It thus resolves #804.

Empirically, the addition of these edges does not change the users' cred
distribution much.  Consider the results with the following 3 forward
weights for the edge (results for ipfs/go-ipfs):

| User          | w=1/32 | w=1/2  | w=2    |
|---------------|-------:|-------:|-------:|
| whyrusleeping | 228.04 | 225.69 | 223.86 |
| jbenet        | 102.04 | 100.26 | 99.53  |
| kubuxu        | 66.60  | 67.80  | 69.36  |
| ...           | —      | —      | —      |
| btc           | 22.69  | 22.29  | 21.38  |

The small effect on users' cred is not that surprising: the
MentionsAuthor references always "shadow" a direct comment->user
reference. In principle, the overall cred going to the user should be
similar; the difference is that now some more cred flows in between the
various comments authored by that user, on the way to the user. (And if
those other comments had references, then it flows out from them, etc.)

Empirically, the variance on comments' scores seems to increase as a
result of having this heuristic, which is great—the fact that all
comments had about the same score was a bug, not a feature.

Sadly, we don't have good tooling for proper statistical analysis of the
effect this is having. We'll want to study the effect of this heuristic
more later, as we build tooling and canonical datasets that makes that
analysis feasible.

We choose to add this heuristic, despite the ambiguous effect on users'
cred, because we think it is principled, and adds meaningful structure
to the graph.

Test plan:
The commit is a pretty straightforward generalization of our existing
GitHub edge logic. All of the interesting logic was thoroughly tested in
the preceding pull, so this commit just tests the integration. Observe
that standard (de)serialization of the edge works, that the snapshot is
updated with a MentionsAuthor reference edge, and that the graph
invariant checker, after update, does not throw errors. Also, I manually
tested this change on the ipfs/go-ipfs repo. (It does not require
regenerating data.)
2018-09-12 12:55:14 -07:00
Dandelion Mané 91f76393e8
Add logic for finding `MentionsAuthorReference`s (#806)
A `MentionsAuthorReference` is created when a post mentions a user, and
that user has authored at least one post in the same thread. Then there
is a `MentionsAuthorReference` from the post to the other posts by that
author.

For context, see the docstrings in `mentionsAuthorReference.js`, and
see #804.

Test plan:
Thorough unit tests have been added, which test the entire pipeline,
from ingesting the data via GitHub's graphql responses, through to
detecting the references. Edge cases such as self-reference and
multi-reference are tested.

Thanks to @wchargin for help writing this commit.
2018-09-11 20:27:31 -07:00
Dandelion Mané bf1e85d6f4
Add `MapUtil.pushValue` for maps of arrays (#805)
With some frequency we find ourselves needing to maintain maps whose
values are arrays that we append to. `MapUtil.pushValue` is a utility
method for these cases.

Existing usage in `aggregate.js` has been modified to use the new
function.

Test plan: Unit tests included.
2018-09-11 15:26:54 -07:00
William Chargin 4b0693e2a7
test: invoke Jest in CI mode (#803)
Summary:
CI mode prevents Jest from automatically writing snapshots, and also
causes obsolete snapshots to be an error instead of a warning. This is
consistent with the behavior on Travis, where the `CI=1` environment
variable is set. It should thus be the default when running `yarn test`
(but not `yarn unit`).

Test Plan:
Add a file `src/foo.test.js`:

```js
// @flow
describe("foo", () => {
  it("bar", () => {
    expect("baz").toMatchSnapshot();
  });
});
```

Note that `yarn test` fails with the message, “new snapshot was not
written”.

Revert this change, then re-run `yarn test`; note that it passes,
writing a snapshot.

Then, reinstate this change and delete `src/foo.test.js`. Note that
running `yarn test` fails, due to an obsolete snapshot. Revert the
change again, and watch `yarn test` pass despite the obsolete snapshot.

Finally, remove the snapshot. :-)

wchargin-branch: test-jest-ci
2018-09-06 20:45:16 -07:00
William Chargin 2d4acf62c5
test: check that JS tests describe their filenames (#802)
Summary:
Resolves #800. The newly added test takes about 2ms per file.

Test Plan:
Run `yarn sharness`, and note that it passes.

Then, edit (say) `src/main/test.js` to change the top-level describe
block from `"cli/main"` to something else, or to remove it altogether.
Re-run `yarn sharness` and note that it fails with a helpful message:

```
test_js_tests_have_top_level_describe_block_with_filename.t .. 1/?
not ok 31 - test file: cli/main.test.js
test_js_tests_describe_filename.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/65 subtests
```

wchargin-branch: describe-test
2018-09-06 20:44:59 -07:00
William Chargin 5fa20ec89e
test: use proper top-level `describe` blocks (#801)
Summary:
Per #800, each test file should start with a `describe` block listing
its file path under `src`. Currently, nine of our tests do not do so.
Of these, eight had a top-level describe block with the wrong name
(either not a filepath or an outdated filepath), while only one short
test was missing a top-level describe block altogether. This patch fixes
each file to use the correct format.

Test Plan:
Apply the Sharness test in #802, and note that it fails before this
patch but passes after it.

wchargin-branch: describe-fix
2018-09-06 20:39:46 -07:00
Dandelion Mané 508fbc5d72
Release 0.1.0 (#799)
Test plan: I ran `yarn test --full`. I also regenerated data from
scratch and manually tested the cred explorer.
2018-09-06 19:06:16 -07:00
Dandelion Mané bf35bbbbda
Move WeightConfig into PagerankTable (#798)
Previously, the WeightConfig (and the button that expanded it) were in
the credExplorer App. This was a little weird, as there's no reason to
play with the weights before you have some Pagerank results to
investigate; additionally, it risked confusing new users with a concept
that was not yet applicable.

Also, the implementation was wonky: the WeightConfig had responsibility
for expanding/hiding itself, which gave poor ability to position the
button and the WeightConfig separately.

Finally, the codepath was untested (vestiges of #604).

This commit fixes all three issues:
- The WeightConfig and button have moved into PagerankTable
- The WeightConfig is now a stateless component, and the parent takes
responsibility for deciding when to mount it
- Logic for showing/hiding the WeightConfig is now tested.
2018-09-06 18:58:09 -07:00
Dandelion Mané b632bd6188
Move `WeightConfig` into the `weights` directory (#797)
Test plan: `yarn test` sufficies for this simple move.
2018-09-06 17:29:15 -07:00
Dandelion Mané eb065f3634
Make `credExplorer/App` control the WeightedTypes (#796)
This commit implements a [suggestion] to make `credExplorer/App` a
single source of truth on the `WeightedTypes`. As such, both
`WeightConfig` and `PluginWeightConfig` have been refactored to be
(essentially) stateless components that are controlled from above. I say
essentially because `WeightConfig` still has its expanded state, but
that will go away soon.

Along the way, I've improved testing and added some new invariant
checking (e.g. that `PluginWeightConfig` was passed the correct set of
weights for its adapter). For the first time, there are now tests for
the `WeightConfig` itself! I'm not totally done with the weight
re-write, but this seems like a good time to close #604, as the whole
logical sequence for setting weights is now tested.

Test plan: There are new unit tests. Also, to be sure, I manually tested
the UI as well.

[suggestion]: https://github.com/sourcecred/sourcecred/pull/792#issuecomment-419234721
2018-09-06 17:17:57 -07:00
Dandelion Mané ead0157960
Change WeightedTypes to contain maps (#795)
This will make it easier to re-organize the weight components so that
the WeightedTypes have a single source of truth, as described in
https://github.com/sourcecred/sourcecred/pull/792#issuecomment-419234721

Test plan: Unit tests suffice.
2018-09-06 16:58:40 -07:00
Dandelion Mané ad5ea761ea
`credExplorer/App` stores weights, not evaluator (#792)
This commit refactors `credExplorer/App` so that instead of storing an
`EdgeEvaulator` in its state, it stores `WeightedTypes` instead. This
has a few benefits:

- It's trivial to generate the right default value for `WeightedTypes`,
so we no longer allow the variable to be nullable in the state. This
simplifies logic, removes an error case, and means that we don't require
the `WeightConfig` to mount before the app is usable.
- `WeightedTypes` are serializable and can be tested for equality, so
they are a better-behaved piece of state
- We put off the information-destroying transformation as long as
possible
- In the future, I think we may want to move the weights/types concept
into core, at which point the `WeightedTypes` will directly be consumed
by the `core/attribution` module.

Test plan: Unit tests are pretty thorough; to be safe, I tested the UI
myself.
2018-09-06 15:29:38 -07:00
Dandelion Mané 417265a4d4
Add `MapUtil.merge` for combining maps (#794)
See the docstring for details.

Test plan: Unit tests.
2018-09-06 14:53:49 -07:00
William Chargin 513820c177
deps: upgrade `flow-bin@^0.80.0` (#791)
Summary:
This upgrade didn’t require fixing any new errors, but Flow is a good
dependency to keep on top of.

Test Plan:
Running `yarn flow` suffices.

wchargin-branch: flow-v0.80.0
2018-09-06 13:37:29 -07:00
William Chargin 0b4fea1c4f
flow: add typing to Jest transform modules (#790)
Test Plan:
`yarn flow` passes, and the [Jest docs][1] suggest that the added type
annotations are correct.

[1]: https://jestjs.io/docs/en/configuration#transform-object-string-string

wchargin-branch: flow-jest-transforms
2018-09-06 13:30:33 -07:00
William Chargin 92514ad559
deps: remove some unused packages (#789)
Summary:
Mostly Webpack loaders that have become unused through various config
changes.

Test Plan:
Check that these packages are not used anywhere except as transitive
dependencies:

```shell
$ git show --format= package.json |
>     sed '1,4d' | grep '^-' | cut -d\" -f2 | git grep -cf -
yarn.lock:3
```

Also, `yarn && yarn test --full` works, and `yarn start` works, and
`yarn backend && node ./bin/sourcecred.js load sourcecred/example-git`
works.

wchargin-branch: remove-unused-deps
2018-09-06 12:08:45 -07:00
Dandelion Mané c2920547c0 Refactor PluginWeightConfig (#788)
This refactors PluginWeightConfig so that it uses the
`defaultWeightsForAdapter` method introduced in #787.

The refactor is mildly invasive, as we switch the state from being a
(mutable) `WeightedTypes` to having a (regular, read-only)
`WeightedTypes`. I think this is an improvement in consistency.

Test plan: Trivial refactor; unit tests+flow pass.
2018-09-05 17:39:21 -07:00
Dandelion Mané b40c1d078d
Create a `weights` module with types and utils (#787)
This commit creates a central `weights` module that defines all of the
weight-related types, and provides some utilities for dealing with them.

This way users of weight-concepts do not need to depend on a lot of
random modules just to get the relevant types. The utility methods are
implicitly defined a few places in the codebase: now we can avoid
re-writing them, and test them more thoroughly.

Test plan: Unit tests pass.
2018-09-05 17:34:29 -07:00
Dandelion Mané d77c76082d
credExplorer gets adapters from one point (#786)
Currently, the `credExplorer` uses the `defaultStaticAdapters`, but it
imports these adapters in multiple places. If we decide to make the
adapters configurable (e.g. when we start supporting more plugins) this
will be a problem.

This change modifies the cred explorer so that the adapters always come
from a prop declaration on the app. Then the adapters are passed into
the `state` module's functional entry points, rather than letting
`state` depend on the default adapters directly.

This change is motivated by the fact that my WeightConfig cleanup can be
done more cleanly if the adapters are present as a prop on the App.

Test plan: Unit tests are updated. Also, `git grep
defaultStaticAdapters` reaveals that the adapters are only consumed
once.
2018-09-05 16:10:11 -07:00
Dandelion Mané f7383bbc90
Add `weightsToEdgeEvaluator` (#785)
This commit adds `weightsToEdgeEvaluator`, a function for converting
weighted node types into an `EdgeEvaluator`. This replaces the
`edgeWeights` module (which was untested, and an outmoded API).

Test plan: The new `weightsToEdgeEvaluator` method is well-tested.
Since `WeightConfig` is still not tested, I manually verified that it
still works as anticipated.
2018-09-05 13:40:40 -07:00
William Chargin 3dda4ab35c
test: invoke `yarn backend` only once (#784)
Summary:
Lots of tests need the output of `yarn backend`. Before this commit,
they tended to create it themselves. This was slow and wasteful, and
also could in principle have race conditions (though in practice usually
tended not to).

This commit updates tests to respect a `SOURCECRED_BIN` environment
variable indicating the path to an existing directory of backend
applications.

Closes #765.

Test Plan:
Running `yarn test --full` passes.

Prepending `echo run >>/tmp/log &&` to the `backend` script in
`package.json` and running `yarn test --full` results in a log file
containing only one line, indicating that the script really is run only
once.

wchargin-branch: deduplicate-backend
2018-09-05 12:47:54 -07:00
Dandelion Mané bd7b7dec82
Export fallback adapter's node and edge types (#783)
This is convenient for testing other code, where we may want to directly
use the fallback types. One test has been updated in this way.

I also changed the names for the fallback adapter's edges to be somewhat
more readable.

Test plan: Tests improved.
2018-09-05 12:34:32 -07:00
William Chargin c48597651e
backend: invoke Webpack directly in package script (#782)
Summary:
This commit removes the `config/backend.js` script and replaces it with
a direct invocation of Webpack. This enables us to use command-line
arguments to Webpack, like `--output-path`.

Test Plan:
Note that `rm -rf bin; yarn backend` still works, and that the resulting
applications work (`node bin/sourcecred.js load`). Note that `yarn test`
and `yarn test --full` still work.

wchargin-branch: backend-webpack-direct
2018-09-05 12:28:27 -07:00
William Chargin ff2cfd14b2
backend: set Babel flags in Webpack config (#781)
Summary:
We currently configure the Babel config with environment variables: in
particular, the `SOURCECRED_BACKEND` environment variable causes Babel
to target Node instead of the browser. The relevant lines are copied
from `scripts/backend.js`.

The environment variable mechanism is slightly clunky, especially as it
requires the Webpack config module to be impure, but it works okay for
our purposes. We could adopt a more principled solution—setting the
`options` argument to the Babel loader in the backend Webpack config—but
this would require redesigning the Babel config system, which would take
a moderate amount of effort.

Test Plan:
As of this commit, `yarn backend` has bitwise identical output to
directly invoking Webpack:

```shell
$ yarn backend >/dev/null 2>/dev/null
$ shasum -a 256 bin/* | shasum -a 256
c4f7494c3ba70e5488ff4a8b44550e478a2a8b27fa96f286123f9566fd28f7be  -
$ NODE_ENV=development node ./node_modules/.bin/webpack \
> --config ./config/webpack.config.backend.js >/dev/null 2>/dev/null
$ shasum -a 256 bin/* | shasum -a 256
c4f7494c3ba70e5488ff4a8b44550e478a2a8b27fa96f286123f9566fd28f7be  -
```

wchargin-branch: backend-set-babel-flags
2018-09-05 12:10:19 -07:00
William Chargin 98a039d340
backend: use Flow in Webpack config (#780)
Summary:
Over the past few commits, I have accidentally removed all Flow errors
from the Webpack config. We can now use Flow on that file to prevent any
new errors from creeping in.

Test Plan:
Running `yarn flow` suffices.

wchargin-branch: backend-flow
2018-09-05 12:03:30 -07:00
William Chargin 4e8c89abfe
backend: export a normal Webpack config object (#779)
Summary:
Previously, our `webpack.config.backend.js` file actually exported a
function that could be used to make a Webpack configuration object.
(This is not to be confused with the late `makeWebpackConfig.js`, which
actually exported a configuration object!)

In addition to being confusing nomenclature, this was a sneaky trap for
CLI users. Invoking `webpack --config config/webpack.config.backend.js`
would actually work, but do the wrong thing: Webpack _allows_ your
configuration object to be a function, but with different semantics. In
particular, the result was that Webpack would emit the build output into
your current directory instead of into `bin/`.

This commit fixes that by making `webpack.config.backend.js` export the
Webpack configuration object for the backend JavaScript applications.
The logic to change the path is now handled by the caller, by
overwriting `config.output.path`; this is exactly [the same approach
that the Webpack CLI takes when given an `--output-path`][1], so it’s
okay with me.

[1]: 368e2640e6/bin/convert-argv.js (L406-L409)

Test Plan:
Run `yarn backend` and `yarn backend --dry-run`. Note that each runs
with appropriate output (both emitted files and console logs).

wchargin-branch: backend-webpack-config-object
2018-09-05 11:58:20 -07:00
Dandelion Mané 2779770af5
Organize weights by plugin (#773)
This commit adds PluginWeightConfig, which is responsible for
adding all the weights for an individual plugin. The top-level
WeightConfig now creates multiple PluginWeightConfigs. It also takes
responsibility for hiding the FallbackPlugin.

Test plan: The PluginWeightConfig is tested (and fairly simple). The
top-level WeightConfig is not yet tested (#604), so I manually tested
that the weights in the app still function.
2018-09-05 11:57:20 -07:00
William Chargin 846363465d
backend: remove file size measurement logic (#778)
Summary:
This logic isn’t currently useful to us. If we want this functionality,
we should consider using a Webpack plugin like `size-limit`. In the
meantime, this removes functionality from `scripts/backend.js`,
continuing on the path to #765.

Recommend reviewing with `-w`.

Test Plan:
Run `yarn backend` and `yarn backend --dry-run`, noting that each works.

wchargin-branch: backend-remove-file-sizes
2018-09-05 11:41:00 -07:00
William Chargin 7e66886a70
dep: remove `eslint-loader` (#777)
Summary:
As of #775, this is no longer used.

Test Plan:
A `git grep eslint-loader` shows no results, and `yarn test --full`
passes.

wchargin-branch: remove-eslint-loader
2018-09-05 11:33:47 -07:00
William Chargin c8175b9e3c
backend: remove superfluous file existence check (#774)
Summary:
Webpack will fail (quickly) if any required entry points do not exist.
The `scripts/backend.js` script superfluously checks this, too. This
patch removes that check.

Test Plan:
In `config/paths.js`, change `src/cli/main.js` to `src/cli/wat.js`.
Then, `yarn backend`, and note that Webpack fails quickly, with an error
“entry module not found”. Note that Webpack fails equally quickly if you
change the path of the last entry point rather than the first one.

wchargin-branch: backend-remove-exists-check
2018-09-05 11:26:22 -07:00
William Chargin 3d0f295ba7
webpack: remove superfluous linting step (#775)
Summary:
We lint separately, with `yarn lint`. There’s no need to duplicate this
effort.

Test Plan:
Introduce a lint error, for instance by adding `("unused expression");`
to `src/cli/main.js` and `src/app/App.js`. Note that `yarn lint` fails
but `yarn backend` and `yarn start` and `yarn build` succeed.

wchargin-branch: webpack-no-lint
2018-09-05 11:25:59 -07:00
William Chargin 8c0bbbc732
backend: move build dir removal into Webpack (#776)
Summary:
Both the backend and the web builds want to empty the build directory
before starting. This commit makes them use the same codepath, reducing
the amount of work that `scripts/backend.js` does so that we can more
easily remove it (#765).

Test Plan:

```shell
$ touch ./bin/wat
$ yarn backend >/dev/null 2>/dev/null
$ file ./bin/wat
./bin/wat: cannot open `./bin/wat' (No such file or directory)
```

wchargin-branch: backend-empty-backend-build-directory
2018-09-05 11:25:52 -07:00
Dandelion Mané f191995c61
Fail on console errors when testing enzyme (#769)
`testUtil.configureEnzyme` now additionally asserts, after every test,
that `console.error` and `console.warn` were not called. Tests that
explicitly expect such calls can still be written by manually re-mocking
the relevant console method (and several examples already exist).

The code that explicitly specifies this for various enzyme test files
has been removed.

Test plan: `git grep "not.toHaveBeenCalled"` shows only unrelated usage.
`yarn test` passes. Adding a spurious console.warn to a passing test
causes it to fail.

Fixes #668
2018-09-05 11:12:38 -07:00
William Chargin d2727c01ba
Fix insidious quoting bug in build test script (#772)
Summary:
This patch fixes a particularly sneaky bug. Our test script contains a
literal backtick inside single quotes. This is generally not a problem,
because backticks inside single quotes do nothing. But the contents of
the single quotes are interpreted as Bash by our test runner, and at
that time the single quotes are expanded to a command substitution.
Therefore, `grep` is invoked as if writing

    grep -e "warning: running $(yarn backend)"

at the CLI. This will actually invoke `yarn backend`!

The magnificent aspect of this bug is that it both makes the test script
slower by about ten seconds _and_ completely and silently defeats the
assertion in which it’s contained. The output of `yarn backend` contains
several blank lines. Therefore, one of the literal patterns to `grep`
contains a blank line. This causes `grep` to match _every_ line in the
error file, regardless of whether it is one of the intended messages.

This patch is the 666th PR to SourceCred. In my opinion, it deserves
this dubious honor.

Test Plan:
Note that `yarn test --full` works, but fails if one of the expected
error message patterns is deleted or munged.

Confirm the behavior by prepending `echo backend >>/tmp/log &&` to the
`yarn backend` script in `package.json`, noting that the resulting log
file contains four lines before this patch and two lines after it.
(Don’t forget to delete/clear the log file before invocations.)

Confirm the behavior of `grep` by writing:

```shell
$ printf 'things went wrong!\n' >err
$ printf 'wat\n\nwot\n' >patterns
$ grep -vF -e "okay" -e "warn: `cat patterns`" err; echo $?
1
$ printf 'wat\nwot\n' >patterns  # no empty line
$ grep -vF -e "okay" -e "warn: `cat patterns`" err; echo $?
things went wrong!
0
```

wchargin-branch: fix-build-test-quoting
2018-09-05 10:53:05 -07:00
William Chargin 1816b13525
Fix expected error message in build test (#771)
Summary:
This change should have happened in #768. However, I didn’t catch it
then because `yarn test --full` passes even before this commit, despite
the expected error being clearly wrong! It turns out that a very sneaky
bug conspires with this one to result in the test passing no matter what
kinds of warnings `yarn backend` may output. This bug is fixed in #772.

Test Plan:
Observe that the error message is now correct by comparing against the
source in `config/RemoveBuildDirectoryPlugin.js`. Then, apply #772 and
note that `yarn test --full` still passes, but does not pass when #772
is applied and this change is reverted.

wchargin-branch: fix-expected-error-message
2018-09-05 10:41:53 -07:00
William Chargin 5e0833421a
Make `execDependencyGraph` a CommonJS module (#767)
Summary:
This simplifies interfaces everywhere.

See also #216, which did the opposite of this as a temporary fix due to
a Babel/Webpack interaction that no longer exists as of #766.

Test Plan:
Note that `node bin/sourcecred.js load sourcecred/example-git` still
works (after `yarn backend`). Note that `yarn test` still works. These
demonstrate that the module works from both a Webpack context and a Node
context. Note that `git grep --name-only execDependencyGraph` yields
exactly those files touched in this commit. Note that `yarn test --full`
passes.

wchargin-branch: commonjs-execDependencyGraph
2018-09-04 22:01:22 -07:00
William Chargin 9b31905ab4
babel: transform ES6 modules to CommonJS (#766)
Summary:
The approach used by `create-react-app` (the source of this code) seems
to be the following: write everything as ES6 modules (`import`/`export`)
and bundle everything with Webpack—in particular, do not transform
modules with Babel.

This works fine until you want to also access some of these modules from
raw Node. Node only supports CommonJS modules, so these “polyglot”
modules must be CommonJS-compatible. But CommonJS modules are not nicely
compatible with ES6 modules: you need to set `exports.default` instead
of `module.exports` in the provider, and dereference `.default` after
each `require`. Babel will perform these transformations for us if we
ask it to. In this patch, we do so.

Test Plan:
Note that `yarn test --full` passes and `yarn start` still works.
(Follow-up commits will exercise this functionality.)

wchargin-branch: transform-es6-modules
2018-09-04 21:47:36 -07:00
William Chargin ddc93826be
Rename `makeWebpackConfig` to `webpack.config.web` (#770)
Summary:
The distinction was useful while `makeWebpackConfig` was being developed
(between #562 and #570), but is now confusing: we have a web config and
a backend config, and it is clearer if we name them as such.

Test Plan:
All of `yarn start`, `yarn build`, and `yarn test --full` work.

wchargin-branch: webpack-config-web
2018-09-04 21:45:10 -07:00
William Chargin 86c1b2e068
webpack: empty build dir instead of removing it (#768)
Test Plan:
Run `mkdir /tmp/out; cd /tmp/out; python -m SimpleHTTPServer`. In
another shell, run `./scripts/build_static_site.sh --target /tmp/out`.
Then, `curl localhost:8000`. Before this commit, this would have yielded
an `OSError` because the cwd of the Python process had been removed.
As of this commit, it works fine.

Also, run `git grep -c rimraf` and note only `yarn.lock:15`.

wchargin-branch: webpack-empty-build-directory
2018-09-04 21:44:09 -07:00
Dandelion Mané a133a7252f
Fix a bug that can lock the cred explorer UI (#764)
Currently, it's possible to lock the cred explorer UI via the following
sequence of actions:

1. Press the analyze cred button
2. While it is running, modify the weights

After following this repro, the UI is stuck: it will say "loading"
forever, and the analyze cred button is disabled.

The issue is that loadGraph and runPagerank do not apply their success
(or failure) state transitions if they are pre-empted by another state
change. If a repo change occurs, that's the right behavior: the repo
change puts the state back to `"READY_TO_LOAD_GRAPH"`, which means the
UI is ready to re-load, and showing the PageRank results for the wrong
repo would be very confusing.

However, if an edge evaluator change occurs while loadGraph or
runPagerank is happening, the state is left in the "LOADING" status
(which means the analyze cred button  is disabled).

This commit fixes the issue via a refactor: per @wchargin's suggestion,
responsibility for the edge evaluator moves from the state module out to
`credExplorer/App.js`. This dramatically simplifies the state module, as
we no longer need a `Substate` concept: we can simplify the state into a
single sequence of states.

As of the refactor, the bug is impossible.

Test plan: Unit tests have been updated to maintain coverage on the
refactored code. I manually tested that the bug no longer repro's, and
that the cred explorer UI continues to function. I did not add a new
test to protect against regression, because in the new codepath, the bug
is basically impossible.
2018-09-04 19:41:37 -07:00
Dandelion Mané 67c443af99
Add default weights for edge types (#762)
This commit modifies the edge type so that it has a
`defaultForwardWeight` and `defaultBackwardWeight`, and these defaults
are respected by the `WeightConfig`.

I came up with reasonable-seeming defaults for the Git and GitHub
plugins; these will undoubtably be more methodically tuned in the
future.

Test plan: `yarn test` passes, also opening the cred explorer now has
the specified default weights in the WeightConfig. (Note that the
forward/backward directions are reversed as described in #749.)
2018-09-04 16:00:49 -07:00
Dandelion Mané c7e5a3b87d
Configure forward/backward edge weights separately (#749)
This commit introduces a new component, `EdgeTypeConfig`, which is
responsible for configuring the weights for a given edge type. The
config creates two `WeightSlider`s: one for the forward direction, and
one for the backward direction. The `DirectionalitySlider` is no longer
used, and is removed. This fixes #596.

So as to avoid confusion, we now describe every edge with variables, as
in 'α REFERENCES β', and clarify that the weight modifies how cred flows
from β to α. This necessitated the creation of an `EdgeWeightSlider`,
local to the `EdgeTypeConfig`, which sets up a `WeightSlider` with the
necessary greek characters.

The EdgeTypeConfig is tested, so this is continuing progress towards
solving #604.

Test plan: I manually verified that modifying edge weights has the
expected effect on cred scores. Also, some new unit tests are included.
2018-09-04 15:37:00 -07:00
Dandelion Mané 4cd45c77fc
Factor out NodeTypeConfig (#763)
This factors `NodeTypeConfig` out of the `WeightConfig` component. The
scope for a `NodeTypeConfig` is that it configures a single node type.
Right now it just renders a single `WeightSlider`, but I like factoring
out both for consistency with the `EdgeTypeConfig` (see #749) and
because I expect we may want to add more complexity later.

Test plan: The new component has some tests, also I manually tested the
frontend.
2018-09-04 12:52:53 -07:00
Dandelion Mané 44407b5520
Combine loadGraph and runPagerank into one button (#759)
* StateTransitionMachine.loadGraph reports success

Step one towards #586. This will enable us to chain runPagerank after
loadGraph only if the load went through successfully.

Test plan: Unit tests included.

* Add StateTransitionMachine.loadGraphAndRunPagerank

This methods combines `loadGraph` and `runPagerank` into one method
which internally chains the two method. `runPagerank` is only called if
`loadGraph` was successful.

Progress on #586.

Test plan:
The new method has attached unit tests. I implemented the unit tests via
mocking, which seemed quite convenient as the method is basically a
wrapper for chaining two other function calls.

* Combine loadGraph and runPagerank into one button

Resolves #586. The new button is called "Analyze cred".

Test plan: Unit tests, also I tested it manually.
2018-09-03 14:34:14 -07:00