This pull request adds a string type as a field of the address, thus canonicalizing that all Nodes and Edges have a Type. This allows for simpler PluginAdapters, and simpler implementation of the GitHub plugin (as it no longer needs to invent its own mechanism for storing types).
I explored making the Address interface generic, with a Type parameter that is a subtype of string. Unfortunately, Flow type resolution seems to have an exponential performance degradation with many subtyped parameters, and adding the extra type parameters to Graph.merge resulted in my flow instance locking. Maybe we can explore adding the subtypes later.
In the GitHub plugin, we entirely do away with the NodeIDs, but we still have EdgeIDs. I plan to remove the need for EdgeIDs in a separate PR which will enforce uniqueness of (srcAddress, dstAddress, pluginName, type) tuples, so that explicitly generating IDs for edges will be unnecessary. In the mean time, I bifurcated the makeAddress function in the GitHub plugin to makeEdgeAddress and makeNodeAddress.
Test plan:
Flow and unit tests all pass. Inspect snapshots, and verify that all the changes are reasonable. Note that since we order by serialized address, adding the type field to address has changed the snapshot order in a few cases.
Close#103
Summary:
This commit moves our existing frontend tests to use Enzyme’s shallow
rendering API <http://airbnb.io/enzyme/docs/api/shallow.html>. The
benefit over also using `react-test-renderer` is simply consistency (the
two are functionally equivalent); the benefits over `mount` are that
subcomponents cannot contaminate the test state (i.e., you’re only
testing one component at a time), that the resulting snapshots are more
readable because the root props are not shown, and that the
implementation is more efficient. This is a follow-up to #102.
In a case where we actually need a full DOM tree, we should still feel
free to use `mount`, but we haven’t needed that yet.
Test Plan:
Verify that the new `ContributionList.test.js.snap` represents the same
data as the old one.
wchargin-branch: standardize-enzyme-shallow
Summary:
This is our first dynamic test of a React component! Enzyme looks pretty
easy to use to me, for both snapshot tests and interaction simulation.
In doing so, we catch a minor bug in the edge case where a contribution
is not owned by any plugin (`colSpan`, not `colspan`). This edge case
does not appear in the sample data, but it does appear in the test data,
even prior to this commit. The previous renderer, `react-test-renderer`,
appears not to surface this error. Furthermore, this bug did not cause
any user-visible errors except a `console.error`.
Test Plan:
Inspect the snapshot file to make sure that it is reasonable. (The
existing test case has its snapshot regenerated due to formatting
differences between the two renderers.)
To test that the browser error is fixed, render a contribution list on a
GitHub graph but with an empty adapter set. One way to do this is to comment out line 7 of
`standardAdapterSet.js`; alternately, you can use the React Dev Tools to
select the `ContributionList` node, then run
```js
$r.props.adapters.adapters = {};
$r.forceUpdate();
```
Note subsequently that there is no console error and that the `<td>`s in
question span three columns.
wchargin-branch: contributionlist-dynamic-test
Summary:
Filter options are “all contributions” or a specific plugin/type
combination. This includes a snapshot test for the static state. I’ll
add an interaction test in a subsequent commit.
Test Plan:
`yarn start`, fetch graph, play with the filtering options.
wchargin-branch: filter-contributions
* Make GitHub capitalization consistent within code
We now never capitalize the H in GitHub within variable or function
names. We still capitalize it in comments or user facing strings.
Test plan:
Unit tests, the fetchGithubRepoTest.sh, and
`git grep itHub` only shows comment lines and print statements.
* Fix William's klaxon
This commit only adds logic for finding references in GitHub posts,
either by #-numeric reference, or explicit urls. Adding the reference
edges to the graph will occur in a followon commit.
Test plan: New unit tests are included
Summary:
Any tests that render Aphrodite-styled React elements will need to do
this, so it’s nice to have the code in one place.
wchargin-branch: aphrodite-testutils
Summary:
In addition to being nicer on the eyes, this enables the query to be
statically analyzed (e.g., by an auto-pagination API) and used by other
modules.
Test Plan:
Manually running
```shell
$ yarn backend
$ GITHUB_TOKEN="<your_token>" src/plugins/github/fetchGitHubRepoTest.sh
```
succeeds.
wchargin-branch: use-structured-graphql-queries
Summary:
This commit begins to extend the artifact editor to display
contributions. To display contributions from arbitrary plugins, we need
to communicate with those plugins somehow. We do so via an adapter
interface that plugins implement; included in this commit is an
implementation of this interface for the GitHub plugin (partially: we
punt on rendering).
This includes a snapshot test. The snapshot format is designed to be
human-readable and -auditable so that it can serve as documentation.
Test Plan:
Run the application with `yarn start`. Then, fetch a graph and watch as
its contributions appear in the view.
wchargin-branch: contributions-and-adapters
Summary:
This is useful for metaprogramming. For instance, suppose we have an
object like this:
```js
const stringifiers = {
ISSUE: (stringifyIssue: (Node<IssueNodePayload>) => string),
COMMENT: (stringifyComment: (Node<CommentPayload>) => string),
...
}
```
How do we type this? We might try
```js
{[type: NodeType]: (Node<NodePayload>) => string}
```
but this is not correct, because `Node<IssueNodePayload>` is a subtype of
`Node<NodePayload>`, and `(_) => K` is contravariant, not covariant. (In
other words, a function from `Node<IssueNodePayload>` is not as general
as a function from `Node<NodePayload>`.) We need to express a dependency
between the object key and the value. We instead write:
```js
type TypedNodeToStringifier = <T: $Values<NodeTypes>>(
T
) => (node: Node<$ElementType<T, "payload">>) => string;
(stringifiers: $Exact<$ObjMap<NodeTypes, TypedNodeToStringifier>>);
```
This expresses exactly (heh) the right type.
Test Plan:
Note that removing any of the elements of `NodeTypes` yields a Flow
error, due to the static assertion following the type definition.
wchargin-branch: node-types
I added some new issues to sourcecred/example-repo to test unicode
support and parsing of extremely long issues titles.
This commit merely updates our example-repo json and the corresponding
snapshots.
Test plan:
Run testFetchGithubRepo.sh
Run unit tests
Also, since there are now two types of things that are being
"contained" (comments and pull request reviews), I factored out an
addContainment method to avoid repeating that code.
To make our handling of PullRequestReviewComments and regular Comments
consistent, I modified our query string so that we now request urls on
PullRequestReviewComments. Also, since I didn't notice until closely
inspecting the snapshot that we had been adding payloads with some
undefined properties, I added a test to verify that every property on
every node and edge payload is defined.
I regenerated the example-repo data to reflect the change to query
string.
Test plan:
Verify that the snapshot changes are appropriate
Run standard tests
Run `yarn backend`
Run `GITHUB_TOKEN={your_token} ./src/plugins/github/fetchGithubRepoTest.sh`
Summary:
This is quick and dirty. No error handling yet. We’ll soon save
credentials and repository to local storage.
Paired with @dandelionmane.
Test Plan:
Run `yarn start`, then enter your API key and specify the
`sourcecred/example-repo` repo. Note that the resulting graph is shortly
logged to the console.
wchargin-branch: fetch-parse-github-frontend
Summary:
We’ll now start creating the artifact plugin. A large part of this will
be the user interface, including a GUI. For now, our build system just
builds a single React app, so we’re cannibalizing the main explorer to
serve this purpose.
Paired with @dandelionmane.
Test Plan:
The following still work:
- `yarn test`
- `yarn start`
- `yarn build; (cd build; python -m SimpleHTTPServer)`
wchargin-branch: repurpose-react-app-as-artifact-editor
Summary:
We’re not deleting it because it works with the build system and has the
service worker stuff from create-react-app, but we’ll soon repurpose it.
Paired with @dandelionmane.
Test Plan:
The following still work:
- `yarn test`
- `yarn start`
- `yarn build; (cd build; python -m SimpleHTTPServer)`
wchargin-branch: dismantle-explorer
The GitHub parser transforms GraphQL api data from GitHub into our Graph
data structure. This commit focuses on properly parsing Issues, Pull
Requests, Comments, and Users.
Test Plan:
Run the unit tests. Inspect the snapshot results (particuarly those for
individual pull requests or issues, which are easier to parse) and
verify that the output is appropriate.
Summary:
Running `yarn backend` will now bundle backend applications. They’ll be
placed into the new `bin/` directory. This enables us to use ES6 modules
with the standard syntax, Flow types, and all the other goodies that
we’ve come to expect. A backend build takes about 2.5s on my laptop.
Created by forking the prod configuration to a backend configuration and
trimming it down appropriately.
To test out the new changes, this commit changes `fetchGitHubRepo` and
its driver to use the ES6 module system and Flow types, both of which
are properly resolved.
Test Plan:
Run `yarn backend`. Then, you can directly run an entry point via
```
$ node bin/fetchAndPrintGitHubRepo.js sourcecred example-repo "${TOKEN}"
```
or invoke the standard test driver via
```shell
$ GITHUB_TOKEN="${TOKEN}" src/backend/fetchGitHubRepoTest.sh
```
where `${TOKEN}` is your GitHub authentication token.
wchargin-branch: webpack-backend
Summary:
See motivation in #76. Feel free to look at the new snapshot file to
inspect the structured representation and also the stringified output.
This implementation is sufficient to encode our query against the
GitHub v4 API; see the test plan below.
Test Plan:
Unit tests added; run `yarn flow && yarn test`.
This code has full coverage except for lines 260, 315, and 380 of
`queries.js`; these lines check invariants that should never be
violated.
You can also use the following steps to verify that the sample query is
valid GraphQL that produces the same results as our hand-written query:
1. Apply the following hacky patch:
```diff
diff --git a/src/backend/graphql/queries.test.js b/src/backend/graphql/queries.test.js
index 52bdec7..c04a636 100644
--- a/src/backend/graphql/queries.test.js
+++ b/src/backend/graphql/queries.test.js
@@ -3,6 +3,18 @@
import type {Body} from "./queries";
import {build, stringify, multilineLayout, inlineLayout} from "./queries";
+function emitGitHubQuery(layout, filename) {
+ const fs = require("fs");
+ const path = require("path");
+ const result = stringify.body(usefulQuery(), layout);
+ const outputFilepath = path.join(__dirname, "..", filename);
+ const outputText = `module.exports = ${JSON.stringify(result)};\n`;
+ fs.writeFileSync(outputFilepath, outputText);
+ console.log(`Wrote output to ${outputFilepath}.`);
+}
+emitGitHubQuery(multilineLayout(" "), "githubQueryMultiline.js");
+emitGitHubQuery(inlineLayout(), "githubQueryInline.js");
+
describe("queries", () => {
describe("end-to-end-test cases", () => {
const testCases = {
```
2. Run `CI=true yarn test`, and verify that the following two files
written to `src/backend/` contain appropriate contents. You can just
eyeball them, or check that they match my results:
https://gist.github.com/wchargin/f37b99fd4ec345c9d2541c2dc53ceda9
3. In `fetchGitHubRepo.js`, change the definition of `const query` to
```js
const query = require("./githubQueryMultiline.js");
```
Run
```shell
GITHUB_TOKEN="<your_token_here>" src/backend/fetchGitHubRepoTest.sh
```
and verify that it exits successfully.
4. Repeat for `require("./githubQueryInline.js")`.
wchargin-branch: graphql-structured-queries
Summary:
Closes#82. This affords clients type-safety without needing to
verbosely annotate every node or edge passed into the graph functions.
It also enables graph algorithms to be more expressive in their types:
for instance, the merge function now clearly indicates from its type
that the first graph’s nodes are passed as the first argument to the
node reducer, and the second graph’s nodes to the second. Clients can
upgrade immediately by using `Graph<*, *>`.
Thankfully, Flow supports variance well enough for this all to be
possible without too much trouble.
Test Plan:
Existing unit tests pass statically and at runtime. I added a test case
to demonstrate that merging works covariantly.
To see some failures, change `string` to an incompatible type, like
`number`, in the definitions of `makeGraph` in test functions for
conservatively rejecting graphs with conflicting nodes/edges
(ll. 446, 462).
wchargin-branch: parameterize-graph
Summary:
Flow doesn’t allow us to specify variance annotations in generic
function parameters, and doesn’t allow coercing `Node<T>` to
`Node<mixed>`. This forces us to put `any`s in our code, which…works.
Paired with @dandelionmane.
Test Plan:
New unit tests trivially pass dynamically, and now pass statically
(failing before the changes to `graph.js`).
wchargin-branch: explicitly-typed-nodes-edges
Graph.addNode and Graph.addEdge now allow adding the same node or edge
multiple times, provided that the duplicate adds are trying to insert
identical content.
This came up while prototyping the GitHub plugin; rather than create
myriad subgraphs and merge them, I found it convenient to construct a
single graph and iteratively add nodes. Since the same node may be
discovered multiple times (most notably user identities), there was a
need for a "conservative add" abstraction that adds a node if it doesn't
exist yet, but errors only if multiple adds conflict.
Since this behavior is generic and highly conservative, it seemed
appropriate to include in the graph class itself.
Test Plan:
The unit tests have been updated to include the new behavior.
I moved sourcecred/tiny-example-repository to sourcecred/example-repo
as it's simpler to remember. I also unarchived it and added comments
to an issue, so that we can create a simple test for issue parsing.
This commit merely updates SourceCred to point to example-repo with
the regenerated canoncial output.
Summary:
It’s a whole new world of GraphQL! Our parser is now just a GraphQL
query that asks for exactly what we want and dumps it to a file. The
data exposed by the v4 API is also in a much nicer format than that of
the v3 API, so this is pretty much a universal improvement.
Currently, we do not handle pagination. We require that the repository
in question have fewer than a fixed number of issues, and comments per
issue, and reviews per PR, and review comments per PR, and so on. If
this limit is exceeded, the script will fail-fast with a nice error
message. To fix this, we’ll need to write a general-purpose pagination
API that allows traversing cursors at any level of the query.
Paired with @wchargin.
Test Plan:
Run
$ GITHUB_TOKEN="your_token_here" src/backend/fetchGitHubRepoTest.sh
and verify that it exits with 0. Note that if you change this script’s
repository from `tiny-example-repository` to `sourcecred`, the script
correctly fails and outputs a useful diff.
wchargin-branch: github-v4-graphql
* Factor evertide graph demo data to a new module
It would be helpful to make our standard tiny graph available to other
test and demo instances, outside of just graph.test.js. This way we can
use it as a test case for the Graph Explorer.
* Make App.js into skeleton for GraphExplorer
We make a very basic skeleton for the Graph Explorer as a basis
for future development.
This commit also removes the UserExplorer and FileExplorer from
App.js. Since we have changed the underlying data model, we are
unlikely to use the UserExplorer or FileExplorer in anything like
their current state, so they are effectively deprecated. I am deferring
removing them because it is nice to have some examples of working React
code to copy from, before the Graph Explorer is ready.
Test plan: run `yarn start`, and observe that the App displays the
words "Graph Explorer" underneath the "SourceCred Explorer" title bar.
Summary:
This commit adds `toJSON()` and `static fromJSON()` on `Graph`. The main
benefit at this time is that this gets us free interoperability with
Jest’s snapshot testing.
The implementation of `fromJSON` is not performance-tuned, and could
probably be significantly optimized.
See #65 for discussion.
Test Plan:
New unit tests added: `yarn flow && yarn test`.
wchargin-branch: make-graph-serializable
Summary:
This commit simplifies the implementation of `Graph` without changing
its interface. We now use the `AddressMap` for all four instance fields
of `Graph`.
Test Plan:
All existing tests pass, and coverage is maintained.
wchargin-branch: use-address-map-in-graph
Summary:
This commit reifies the concept of an `Addressable`, which is any object
that has a covariant `address: Address` attribute, and implements a
simple data structure for storing addressable items keyed against their
addresses. Instances of `AddressMap` can replace the four fields of
`Graph`:
```js
_nodes: AddressMap<Node<mixed>>;
_edges: AddressMap<Edge<mixed>>;
_outEdges: AddressMap<{|+address: Address, +edges: Address[]|}>;
_inEdges: AddressMap<{|+address: Address, +edges: Address[]|}>;
```
Test Plan:
New unit tests included, with 100% coverage: `yarn flow && yarn test`.
wchargin-branch: address-map
Summary:
We’re stripping down the payload types for the GitHub plugin, to only
include what we expect to use immediately. In doing so, we take the
opportunity to make the typing a little stronger, so that we can ensure
that the `type` field of a specific type of payload is set to a
particular constant.
Paired with @dandelionmane.
Test Plan:
Adding these lines to `githubPlugin.js` and running `yarn flow`
indicates that the typechecking is working as expected:
```js
("ISSUE" : NodeType); // works
("WEIRD" : NodeType); // fails
("AUTHORSHIP" : EdgeType); // works
("UNEXPECTED" : EdgeType); // fails
```
wchargin-branch: github-plugin-payload-types
Summary:
`graph.js` coverage is now 100% :-)
Test Plan:
`yarn jest --env=jsdom --coverage` shows no uncovered lines for
`graph.js`, and no failing tests.
wchargin-branch: coverage-gremlin
Summary:
Merging graphs will be a common operation. At a per-plugin level, it
will often be useful to build up graphs by creating many very small
graphs and then merging them together. At a cross-project level, we will
need to merge graphs across repositories to gain an understanding of how
value flows among these repositories. It’s important that the core graph
type provide useful functions for merging; this commit adds them.
Test Plan:
New unit tests added; run `yarn flow && yarn test`.
wchargin-branch: graph-merge
Summary:
We need this for testing graph equality: deep-equality is not sufficient
because two graphs can be logically equal even if, say, two nodes are
added in different orders.
This commit adds a dependency on `lodash.isequal` for deep equality.
Test Plan:
New unit tests added. Run `yarn flow && yarn test`.
wchargin-branch: graph-equals
Summary:
In merging #54, there was a semantic merge conflict that was not also a
textual merge conflict; this created a failure that only appeared once
that commit was merged.
We propose that to fix this in the future, we only merge commits that
are directly ahead of master.
Test Plan:
This fixes `yarn flow` and `yarn test`.
wchargin-branch: fix-merge-conflict
Summary:
Again: we assume these invariants, so we may as well encode them.
We should just keep in mind that non-Flow users may wantonly violate
these, so we should still code defensively.
wchargin-branch: readonly-exact
Summary:
These will make nicer error functions in cases where static analysis
doesn’t detect the pollution: e.g., a user isn’t using Flow, or an
expression like `arr[0]` introduces an `undefined`.
Paired with @dandelionmane.
Test Plan:
New unit tests added. Run `yarn test`.
wchargin-branch: null-undefined-check
Create an 'advancedMealGraph' test case
The advancedMealGraph will be a grab-all that holds all advanced and
edge behaviors, e.g. the crab-self-referential loop, and the case
where there are multiple directed edges between the same two nodes.
Aggregating them into one test case will make it easier to test more
complex behaviors, like graph merging and serialization, on the
edge case graphs. However, it's still nice to have the simple graph
so that we can test simple things too. The specific tests for edge
case behavior are left mostly unchanged, in that they start from the
simple graph and add just the advanced feature that they want to test.
Summary:
Without these functions, it is not possible to meaningfully operate on
an arbitrary graph.
Paired with @dandelionmane.
Test Plan:
New unit tests included. Run `yarn flow && yarn test`.
wchargin-branch: get-all
Summary:
We’ve realized that `u: Edge<T>` implies `u: Node<T>`. That certainly
wasn’t what we were expecting! We might want something like that
eventually, to capture the fact that valuations are themselves valuable,
but for now the type system should encode the assumptions that we’re
actually making. See also #50.
Paired with @dandelionmane.
wchargin-branch: exact-types
Dandelion Mané
William Chargin