Summary:
In particular, we excluded `bin`, but this was catching non-root
directories named `bin`, too, and so files like
`src/plugins/github/bin/fetchAndPrintGithubRepo.js` were not subject to
prettification. Happily, those files are all pretty enough, anyway.
Test Plan:
Note that mangling the format of `fetchAndPrintGithubRepo.js` prior to
this commit would not cause `yarn check-pretty` to fail, nor would the
manglings be fixed by `yarn prettify`—but that both of these behaviors
are reversed after this commit.
wchargin-branch: prettier-exclude-root-only
Summary:
We want our main repository to include submodules so that we can test
submodule support. Here, we create a repository to be included as a
submodule.
wchargin-branch: example-submodule-repository
Summary:
The `loadRepository` test tries to clean up temporary directories, but
failed to do so because the directories were not empty. The cleanup hook
threw an error, but this error was silenced by Jest due to [a known
bug][1] that was fixed a few days ago. We can fix this by asking `tmp`
to clean up directories even if they are not empty, using the
`unsafeCleanup` option.
[1]: https://github.com/facebook/jest/issues/3266
Test Plan:
While running `watch -n 0.1 'ls /tmp | grep "tmp-.*" | wc -l'`, run
tests. Note that the number increases by five and then drops down again;
before this patch, it would increase by 5 and then stay there.
wchargin-branch: clean-up-tmpdirs
Summary:
A few reasons for this:
1. This _is_ a utility, so it makes sense semantically.
2. This unifies the utilities API; clients like `loadRepository.test`
don’t have to keep around both a `git` and a `gitUtils`.
3. Most importantly, further scripts and tests shouldn’t depend on
`loadRepository` just for `localGit`. Depending on `gitUtils` makes
much more sense.
(Note that `makeUtils` is no longer dependency-injectable, but that’s
okay; I considered this and favored YAGNI on this one.)
Test Plan:
Existing unit tests pass.
wchargin-branch: move-localgit
Summary:
Utilities like `deterministicCommit` provide valuable functionality that
we will want to use in other scripts and perhaps other test cases. It
makes sense to factor these out into utility functions.
Test Plan:
Existing tests pass.
wchargin-branch: git-utils
This commit adds an optional `typeOptions` argument to Graph.getInEdges
and Graph.getOutEdges. The `typeOptions` allow filtering the returned
edges by the type of the edge, and the type of the node that the edge is
connected to. This makes it much easier to use these methods to find
connections that have a certain relationship, e.g. finding the author of
a commit or the comments on an issue.
Test plan:
A new test suite was written that comprehensively tests this behavior,
both for getInEdges and getOutEdges.
In preparation for using type info in the Graph apis, it is helpful to
have richer type info in the graph demo data.
Test plan: Check that the snapshot changes only consist of type changes,
and that CI passes.
Our GitHub parser is implemented via a `GithubParser` class which builds
the GitHub graph. This is a convenient implementation, but an awkward
API. This commit refactors the module so that it exposes a clean `parse`
function, which ingests the GitHub JSON data and returns as completed
graph.
Test plan:
The unit tests have been re-written to use the new public API. All the
snapshots are unchanged, and flow passes. Additionally, I ran `yarn
start` and verified that the GithubGraphFetcher for the Artifact plugin
is still working.
Summary:
We should be able to get the types without depending on the function to
load a Git repo from disk, and in particular without depending on
`child_process`.
Test Plan:
Flow and tests are sufficient.
wchargin-branch: extract-repository-types
There's some context at #127, in which I initially proposed this change.
In addition to the long-term benefits described in #127, there is a
short-term benefit which is that it makes snapshot tests easier to read,
because the GitHub ids are opaque and unreadable, while the GitHub urls
are relatively easy to parse.
This change results in significant snapshot churn.
Once the type was added, flow correctly discovered a bug in
GithubGraphFetcher.js, which resulted in broken graph fetching in the
ArtifactEditor. Oops! / Good work Flow!
I made `ensureNoMorePages` expect the result it is testing to be an
`any`, which is appropriate for how the function is written (i.e. it is
written in a way that is agnostic to the actual result).
The example-repo.json file is regenerated with large diffs due to the
change in indentation level throughout the file.
Test plan:
Sanity check the snapshot (close inspection is unnecessary due to the
simplicity of the code change). Check that CI pases.
This commit adds flow typing for the JSON result from hitting the GitHub
graphql api. We can't prove that the flow typing is correct, but since
the type definition is colocated with the corresponding fragment
definitions, we can hope that maintainers will maintain both together.
We update the parser to consume the new flow types. There are no flow
errors.
Test plan:
Inspect the flowtypes, verify that they correspond to the data in
example-repo.json, and that there are no flow errors.
Summary:
In this newly added module, we load the structural state of a git
repository into memory. We do not load into memory the contents of any
blobs, so this is not enough information to perform any analysis
requiring file diffing. However, it is sufficient to develop a notion of
“this file was changed in this commit”, by simply diffing the trees.
Test Plan:
Unit tests added; `yarn test` suffices. Reading these snapshots is
pretty easy, even though they’re filled with hashes:
- First, read over the commit specifications on lines 69–83 of
`loadRepository.test.js`, so you know what to expect.
- In the snapshot file, keep handy the time-ordered list of commit
SHAs at the bottom of the file, so that you know which commit SHA is
which.
- To verify that the large snapshot is correct: for each commit, read
the corresponding tree object and make sure that the structure is
correct.
- To verify the small snapshot, just check that it’s the correct
subset of the large snapshot.
- If you want to verify that the SHA for a blob is correct, open a
terminal and run `git hash-object -t blob --stdin`; then, enter the
content of the blob and press `<C-d>`. The result is the blob SHA.
To run a sanity-check on a large repository: apply the following patch:
<details>
<summary>Patch to print out statistics about loaded repository</summary>
```diff
diff --git a/config/paths.js b/config/paths.js
index d2f25fb..8fa2023 100644
--- a/config/paths.js
+++ b/config/paths.js
@@ -62,5 +62,6 @@ module.exports = {
fetchAndPrintGithubRepo: resolveApp(
"src/plugins/github/bin/fetchAndPrintGithubRepo.js"
),
+ loadRepository: resolveApp("src/plugins/git/loadRepository.js"),
},
};
diff --git a/src/plugins/git/loadRepository.js b/src/plugins/git/loadRepository.js
index a76b66c..9380941 100644
--- a/src/plugins/git/loadRepository.js
+++ b/src/plugins/git/loadRepository.js
@@ -106,3 +106,7 @@ function findTrees(git: GitDriver, rootTrees: Set<Hash>): Tree[] {
}
return result;
}
+
+const result = loadRepository(...process.argv.slice(2));
+console.log("commits", result.commits.size);
+console.log("trees", result.trees.size);
```
</details>
Then, run `yarn backend` and put the following script in `test.sh`:
<details>
<summary>Contents for `test.sh`</summary>
```shell
#!/bin/bash
set -eu
repo="$1"
ref="$2"
via_node() {
node bin/loadRepository.js "${repo}" "${ref}"
}
via_git() (
cd "${repo}"
printf 'commits '
git rev-list "${ref}" | wc -l
printf 'trees '
git rev-list "${ref}" |
while read -r commit; do
git rev-parse "${commit}^{tree}"
git ls-tree -rt "${commit}" \
| grep ' tree ' \
| cut -f 1 | cut -d ' ' -f 3
done | sort | uniq | wc -l
)
echo
printf 'Running directly via git...\n'
time a="$(via_git)"
echo
printf 'Running Node script...\n'
time b="$(via_node)"
diff -u <(cat <<<"${a}") <(cat <<<"${b}")
```
</details>
Finally, run `./test.sh /path/to/some/repo origin/master`, and verify
that it exits successfully (zero diff). Here are some timing results on
SourceCred and TensorBoard:
- SourceCred: 0.973s via Node, 0.327s via git.
- TensorBoard: 30.836s via Node, 6.895s via git.
For TensorFlow, running via git takes 7m33.995s. Running via Node fails
with an out-of-memory error after 39 minutes, with 10GB RAM and 4GB
swap. See details below.
<details>
<summary>
Full timing details, commit SHAs, and OOM error message
</summary>
```
+ ./test.sh /home/wchargin/git/sourcecred 01634aabcc
Running directly via git...
real 0m0.327s
user 0m0.016s
sys 0m0.052s
Running Node script...
real 0m0.973s
user 0m0.268s
sys 0m0.176s
+ ./test.sh /home/wchargin/git/tensorboard 7aa1ab9d60671056b8811b7099eec08650f2e4fd
Running directly via git...
real 0m6.895s
user 0m0.600s
sys 0m0.832s
Running Node script...
real 0m30.836s
user 0m3.216s
sys 0m10.588s
+ ./test.sh /home/wchargin/git/tensorflow 968addadfd4e4f5688eedc31f92a9066329ff6a7
Running directly via git...
real 7m33.995s
user 5m21.124s
sys 1m5.476s
Running Node script...
FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
1: node::Abort() [node]
2: 0x121a2cc [node]
3: v8::Utils::ReportOOMFailure(char const*, bool) [node]
4: v8::internal::V8::FatalProcessOutOfMemory(char const*, bool) [node]
5: v8::internal::Factory::NewFixedArray(int, v8::internal::PretenureFlag) [node]
6: v8::internal::DeoptimizationInputData::New(v8::internal::Isolate*, int, v8::internal::PretenureFlag) [node]
7: v8::internal::compiler::CodeGenerator::PopulateDeoptimizationData(v8::internal::Handle<v8::internal::Code>) [node]
8: v8::internal::compiler::CodeGenerator::FinalizeCode() [node]
9: v8::internal::compiler::PipelineImpl::FinalizeCode() [node]
10: v8::internal::compiler::PipelineCompilationJob::FinalizeJobImpl() [node]
11: v8::internal::Compiler::FinalizeCompilationJob(v8::internal::CompilationJob*) [node]
12: v8::internal::OptimizingCompileDispatcher::InstallOptimizedFunctions() [node]
13: v8::internal::Runtime_TryInstallOptimizedCode(int, v8::internal::Object**, v8::internal::Isolate*) [node]
14: 0x12dc8b08463d
```
</details>
wchargin-branch: load-git-repositories
# Please enter the commit message for your changes. Lines starting
# with '#' will be kept; you may remove them yourself if you want to.
# An empty message aborts the commit.
#
# Date: Mon Apr 23 23:02:14 2018 -0700
#
# HEAD detached at origin/wchargin-load-git-repositories
# Changes to be committed:
# modified: package.json
# new file: src/plugins/git/__snapshots__/loadRepository.test.js.snap
# new file: src/plugins/git/loadRepository.js
# new file: src/plugins/git/loadRepository.test.js
#
# Untracked files:
# out
# runtests.sh
# src/plugins/artifact/editor/ArtifactSetInput.js
# src/plugins/git/repository.js
# test.sh
# todo
#
github/githubPlugin.js was growing ungainly - it contained two major
pieces: all of the node and edge types, and the GitHub parser. As I
contemplated adding a third major new section of logic (an easy-to-use
api for traversing the GitHub graph, with first class support for
comments, authorship, etc), I found the prospect of adding even more
into that file quite unappealing. So, I have instead split it into three
files:
* github/pluginName.js: Exports the plugin name.
* github/types.js: Exports types of nodes, edges, and their payloads
* github/parser.js: Exports `GithubParser`
No logic has been changed whatsoever - this is purely a rename-refactor.
Test plan:
CI still passes. I manually verified that the Artifact Editor can still
load and display GitHub data.
We have urls for all the author types, so for consistency across GitHub
payloads, I am adding urls to the author paylods.
Test plan:
Check that snapshot changes consist entirely of adding urls, and that
the urls are appropraite.
Add logic to findReferences for finding GitHub username references,
e.g. "Hello, @wchargin!". The API is unchanged.
Test plan:
There are new unit tests that verify this behavior works as expected.
Currently, every type of reference has its own type signature: numeric
references are returned as numbers, url references are a complicated
object containing url parts, and so forth.
Since ultimately the references are just strings, it makes more sense to
treat references as plain strings. This allows a much simpler
implementation of reference edge creation in the GitHub plugin. It also
results in a simpler API for the parseReferences file (it only exports a
single findReferences function).
Test plan:
Verify that the updated tests encode appropriate behavior.
Summary:
This replaces the implementation of a static check from a somewhat
complicated use of higher-order types to a more simple empty-union
assertion, as suggested by jez in “Case Exhaustiveness in Flow”:
https://blog.jez.io/flow-exhaustiveness/
(I know; we’re not using Reason. One step at a time. :-) )
I adapted the implementation a bit because I prefer explicitly disabling
an ESLint warning over a no-op function call; it is not clear from the
latter that the purpose is to suppress a lint warning.
Test Plan:
In `githubPlugin.js`, add `| "ANOTHER"` to the `NodeType` type, and note
a compile-time Flow error on the appropriate line, with a very readable
error message. Note that all unit tests pass, and running the UI on
`sourcecred/sourcecred` yields correct titles for each node type present
(namely, all node types except for `ORGANIZATION` and `BOT`).
wchargin-branch: empty-union-assertion
Keeping the GitHub demo data up-to-date is important, and there isn't
good documentation for how to do that.
This commit adds a short README.md for the demo data, and adds an update
flag to fetchGithubRepoTest.sh that can be used to easily update it.
Test plan:
Modify example-repo.json (e.g. by deleting it entirely). Run
fetchGithubRepoTest.sh -u and confirm that the data was regenerated
without change. Run fetchGithubRepoTest.sh and confirm the test passes.
Note: The end cursor is sensitive to the timezone, which seems to be
cached with the GitHub token. An erroneous switch to Israel timezone
made it into master; this commit reverts back to US/Pacific.
This commit modifies our GitHub graphql query so that we request urls
for all objects (e.g. users, pull requests, pull request review
comments). Some change along these lines is necessary so that we can
correctly represent URL reference edges to e.g. issue comments. (It
might be possible to do without by reverse-enginering from the ids, but
we are resolved to treat ids as opaque).
Strictly speaking, we don't need to collect urls for users, issues, and
pull requests - they are generated via simple schema. However, for
consistency, I think it's better to just take URLs on everything.
Test plan: The example-repo.json has been regenerated. The diffs are as
expected.
Summary:
This commit completes the ad hoc pagination solution described in #117:
we implement pagination specifically for our current query against the
GitHub API. This is done in such a way that reasonable additions to the
query will not be hard to implement—for instance, if we want to fetch
a new kind of field, the marginal cost is at most a bit of extra
copy-and-paste and some modifications to tests. However, we should
certainly still plan to implement the fully automatic pagination system
described in #117.
Running on TensorBoard with the default page limits takes 30–33 seconds
over 7 queries, uses 103 GitHub API points (out of 5000 for any given
hour), and produces a JSON file that is 8.7MB (when pretty-printed).
This all seems quite reasonable to me.
Test Plan:
Extensive unit tests added. The snapshots are quite readable, by design.
For a real-world test, you can `yarn start` the artifact viewer and use
the GUI to fetch the data for tensorflow/tensorboard.
To demonstrate that the fetching process gives the same results
regardless of the page size, follow these steps:
1. In `fetchGithubRepo.js`’s `postQuery` function, insert a new
statement `console.error("Posting query...")` at the beginning. (It
is important to print to stderr instead of stdout.)
2. Run `yarn backend` and then invoke `fetchGithubRepo.js` on a repo
large enough to require pagination, like SourceCred or TensorBoard.
Pipe the result to `shasum -a 256` and note the SHA.
3. In `github/graphql.js`, change the page size constants near the top
of the file. Re-run step 2. The number of queries that are posted
will vary significantly as the page size constants vary, but the
checksum should remain the same.
4. Repeat until satisfied. (I tried three sets of values: the standard
values, the standard values but all divided by 10, and all 5s.)
wchargin-branch: ad-hoc-pagination
Summary:
Once we execute the root query, find continuations, embed the
continuations into queries, and execute the continuation query, we will
need to merge the continuations’ results back into the root results.
This commit adds a function `merge` that will be suitable for doing just
that.
Test Plan:
New unit tests added, with 100% coverage. Run `yarn test`.
wchargin-branch: merge-query-results
Summary:
Per #117, this is a first step toward at writing a pagination API that
specifically targets our current GitHub query. For design details, see
new module docs on `src/plugins/github/graphql.js`.
This commit modifies the core GitHub query and thus the
`example-repo.json` snapshot: we now request `endCursor` fields for all
pagination info, and we request the `id` of the root `repository` field.
The former is obviously necessary. The latter is necessary for the
repository to be consistent with other nodes that offer connections as
fields: we require an ID on the node containing the connection so that
we can have random access to it in a continuation selector.
Test Plan:
Unit tests added. You can also try out the generated continuation
queries for yourself: apply the patch below, run `yarn backend`, and
then run the `fetchGithubRepo.js` script on `sourcecred/sourcecred`.
This will output a nicely formatted query that you can paste directly
into GitHub’s API explorer and execute. (Note that, because this patch
is not fully polished, the query must be run against a repository that
has a continuation for every node type: more pages of issues, PRs,
comments, reviews, and review comments. This is due to an
easy-but-annoying-to-fix bug in the patch, not in the code included in
this commit.)
<details>
<summary>Patch for generating a continuations query</summary>
```diff
diff --git a/src/plugins/github/fetchGithubRepo.js b/src/plugins/github/fetchGithubRepo.js
index 789a20e..418c736 100644
--- a/src/plugins/github/fetchGithubRepo.js
+++ b/src/plugins/github/fetchGithubRepo.js
@@ -6,8 +6,13 @@
import fetch from "isomorphic-fetch";
-import {stringify, inlineLayout} from "../../graphql/queries";
-import {createQuery, createVariables} from "./graphql";
+import {stringify, inlineLayout, multilineLayout} from "../../graphql/queries";
+import {
+ continuationsFromQuery,
+ continuationQuery,
+ createQuery,
+ createVariables,
+} from "./graphql";
/**
* Scrape data from a GitHub repo using the GitHub API.
@@ -66,8 +71,13 @@ function postQuery(payload, token) {
if (x.errors) {
return Promise.reject(x);
}
- ensureNoMorePages(x);
- return Promise.resolve(x);
+ console.log(
+ stringify.body(
+ continuationQuery(Array.from(continuationsFromQuery(x.data))),
+ multilineLayout(" ")
+ )
+ );
+ throw new Error("STOPSHIP");
});
}
diff --git a/src/plugins/github/graphql.js b/src/plugins/github/graphql.js
index 9ea2592..9ead42b 100644
--- a/src/plugins/github/graphql.js
+++ b/src/plugins/github/graphql.js
@@ -39,11 +39,11 @@ import {build} from "../../graphql/queries";
*
* [1]: https://developer.github.com/v4/guides/resource-limitations/#node-limit
*/
-export const PAGE_LIMIT = 100;
-const PAGE_SIZE_ISSUES = 100;
-const PAGE_SIZE_PRS = 100;
-const PAGE_SIZE_COMMENTS = 20;
-const PAGE_SIZE_REVIEWS = 10;
+export const PAGE_LIMIT = 10;
+const PAGE_SIZE_ISSUES = 10;
+const PAGE_SIZE_PRS = 10;
+const PAGE_SIZE_COMMENTS = 3;
+const PAGE_SIZE_REVIEWS = 1;
const PAGE_SIZE_REVIEW_COMMENTS = 10;
/**
@@ -340,6 +340,36 @@ function* continuationsFromReview(
}
}
+/**
+ * Combine continuations into a query.
+ */
+export function continuationQuery(
+ continuations: $ReadOnlyArray<Continuation>
+): Body {
+ const nonces: string[] = continuations.map((_, i) => `_n${String(i)}`);
+ const nonceToIndex = {};
+ nonces.forEach((n, i) => {
+ nonceToIndex[n] = i;
+ });
+ const b = build;
+ const query = b.query(
+ "Continuations",
+ [],
+ continuations.map((continuation, i) =>
+ b.alias(
+ nonces[i],
+ b.field(
+ "node",
+ {id: b.literal(continuation.enclosingNodeId)},
+ continuation.selections.slice()
+ )
+ )
+ )
+ );
+ const body = [query, ...createFragments()];
+ return body;
+}
+
/**
* These fragments are used to construct the root query, and also to
* fetch more pages of specific entity types.
```
</details>
wchargin-branch: ad-hoc-pagination-continuations
Summary:
Any time that we pull fields off a connection object, we may need to
repeat the query for subsequent pages. Therefore, such fragments will be
shared across multiple queries, and also shared within a query if we
need to fetch—say—more issue comments on two or more distinct issues.
This is a perfect use case for fragments.
This commit refactors the GitHub query to be organized in terms of
fragments, without changing the format of the results.
(We also take this opportunity to factor the page limits into
constants.)
Test Plan:
After running `yarn backend`, the `fetchGithubRepoTest.sh` test passes.
wchargin-branch: extract-github-query-fragments
Summary:
Per #117, we want to develop an ad hoc pagination API written
specifically against the query that we use to interact with GitHub.
The pagination logic should be separate from the logic to actually fetch
the repo, but should be colocated with the query itself, so this commit
extricates the query from `fetchGithubRepo.js` into a new module.
Test Plan:
Existing tests pass, including `fetchGithubRepoTest.sh`.
wchargin-branch: extract-github-graphql
Summary:
This was created by re-crawling the GitHub repo via `fetchGithubRepo`,
and then updating Jest snapshots.
Test Plan:
Note that `fetchGithubRepoTest.sh` passes, so the data is now up to
date.
Inspect the snapshot, and note that the only changes are to change login
names from `dandelionmane` to `decentralion`. To do so automatically:
```bash
set -eu
diff_contents() {
git difftool HEAD^ HEAD --extcmd=diff --no-prompt
}
! diff_contents | grep '^<' | grep -vF '"dandelionmane"'
! diff_contents | grep '^>' | grep -vF '"decentralion"'
```
wchargin-branch: decentralion-data
Summary:
For pagination, we’ll want to query against multiple entities of the
same type. GraphQL uses aliases to facilitate this. This commit adds
support for aliases to our GraphQL query DSL.
Test Plan:
Inspect snapshot changes, and note that `yarn flow` and `yarn test`
pass.
wchargin-branch: graphql-aliases
Summary:
This component now maintains a graph of just artifacts and the edges
among them. It owns the state, and notifies its parents of changes with
a callback. We treat the graph objects as properly immutable, copying
them on each change. So far, descriptions are always the empty string.
Test Plan:
Interaction tests added. Run `yarn test`.
wchargin-branch: artifact-graph-editor
Update GitHub plugin to respect two new conventions:
- Node/Edge types are exported as UPPER_CASE_CONSTANTS
- Edge types are always verbs, which can be read as $src verb $dst
Test plan:
Flow passes. Inspect snapshot changes.
Summary:
This commit revises our implementations of node and edge types, and
specifies the semantics for artifact plugin IDs: we create IDs by
slugifying an artifact name and then resolving collisions
Test Plan:
Unit tests added. Run `yarn flow` and `yarn test`.
wchargin-branch: artifact-plugin-node-edge-semantics
Summary:
Wherein we change the semantics to allow\* dangling edges. This is
necessary for plugins that want to update nodes, such as changing a
description or other noncritical field.
\* (It was technically possible before by abusing `merge`, but now you
can just do it.)
Paired with @dandelionmane.
Test Plan:
Extensive tests added. Run `yarn flow` and `yarn test`.
wchargin-branch: allow-removing-from-graph
Summary:
Clients of `Graph` that wish to treat the graph as immutable will
benefit from a `copy` method. We should provide it on `Graph` instead of
asking clients to reimplement it because it affords us the opportunity
to get the type signature right: in particular, copying should allow
upcasting of the type parameters, even though `Graph` itself is
invariant.
Paired with @dandelionmane.
Test Plan:
Unit tests added. Run `yarn flow` and `yarn test`. To check that
downcasting is not allowed, change the types in the new static test case
in `graph.test.js` to be contravariant instead of covariant, and note
that `yarn flow` fails.
wchargin-branch: graph-copy
Summary:
We need to know the repo owner and name for purposes other than fetching
the GitHub graph: for instance, fetching the `artifacts.json` file that
describes the artifact subgraph. It makes sense that these should be
settings global to the application. This commit separates a settings
component and the original GitHub graph fetcher.
This invalidates localStorage; you can manually migrate.
Paired with @dandelionmane.
Test Plan:
Note that the data continues to be stored in localStorage and that it is
updated on each keypress. Note that the state is properly passed around:
if you change the repository name from `example-repo` to `sourcecred`,
e.g., and click “Fetch GitHub graph”, then the proper graph is fetched.
wchargin-branch: separate-artifact-settings
* Generate Edge ids automatically
Adds edgeID in graph, which creates a string id from src and dest.
Provided that the plugin only uses edgeID for generating edge ids of
that type, these ids will be unique.
Modify the GitHub plugin to use edgeID. This allows the code and
typesignature to be simpler, and will be more consistent with other
plugins.
Test plan:
Carefully inspect the snapshots.
* Add a toString and fromString method on addresses
The toString and fromString methods use json-stable-stringify,
and we've modified other address code to use these methods.
As such, a number of the snapshots have changed (ordering).
Test plan:
Verify the new included unit tests are comprehensive. Inspect the
snapshots.
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
William Chargin
Dandelion Mané
