Summary:
The public method `checkInvariants` on graph is now cached. The cache is
invalidated when the graph is modified via the public API. As a result
of this change, the time of `yarn ci-test --testPathPattern src/v3/`
decreases from 5.631s to 3.866s (best-of-three timing, but low variance
anyway). This effect becomes much more pronounced as higher-level APIs
check their own invariants by themselves indirectly invoking the graph’s
`checkInvariants` method many times.
Test Plan:
Existing unit tests have been adapted and extended. Tests for the
invariant checking have been updated to call the internal, uncached
method, and new tests have been added to check that the caching behavior
is correct.
wchargin-branch: cache-graph-invariants
This commit:
- adds `github/createGraph.js`
- which ingests GitHub GraphQL response
- and creates a GitHub graph
- adds `github/graphView.js`,
- which takes a Graph
- and validates that all GitHub specific node and edge invariants hold
- every github node may be parsed by `github/node/fromRaw`
- with the right node type
- every github edge may be parsed by `github/edge/fromRaw`
- with the right edge type
- with the right src address prefix
- with the right dst address prefix
- every child node has exactly one parent
- of the right type
- and provides convenient porcelain methods for
- finding repos in the graph
- finding issues of a repo
- finding pulls of a repo
- finding reviews of a pull
- finding comments of a Commentable
- finding authors of Authorables
- finding parent of a ChildAddress
- tests `createGraph`
- via snapshot testing
- by checking the GraphView invariants hold
- tests `graphView`
- by checking individual entities in the example-git repository have
the proper relationships
- by checking that for every class of invariant, errors are thrown if
the invariant is violated
Test plan:
- Extensive unit and snapshot tests added. `yarn travis` passes.
Summary:
This is modeled after the GitHub edge module format. In particular, the
whole length encoding garbage is directly copied. As in that module, we
decline to test the error paths.
Test Plan:
Unit tests added; run `yarn travis`. Snapshots are readable.
wchargin-branch: git-v3-edges
Summary:
This is modeled after the GitHub node module format, with the obvious
alterations plus a bit more type safety in the implementation of `toRaw`
(namely, we check `type` exhaustively).
Test Plan:
Unit tests added; run `yarn travis`.
wchargin-branch: git-v3-nodes
Summary:
Many files are unchanged. Some files have had paths updated, or new
build/test targets added.
The `types.js` file includes payload type definitions. These are
technically independent of the graph abstraction (i.e., nothing from V1
is imported and the code all still works), but it of course implicitly
depends on the V1 model. For now, we include the entirety of this file,
just so that we have a clean copy operation. Subsequent commits will
strip out this extraneous code.
Suggest reviewing with the `--find-copies-harder` argument to Git’s
diffing functions.
Test Plan:
Running `yarn travis --full` passes. Running
./src/v3/plugins/git/demoData/synchronizeToGithub.sh --dry-run
yields “Everything up-to-date”.
wchargin-branch: git-v3-copy
Summary:
We had `edgeExamples`, wherein we constructed examples of edge addresses
(not actual edges) by manual instantiation. We checked that these
matched snapshots, and then we also called `createEdge` a bunch to
create actual edges, and checked that those matched snapshots, too.
Consequently, we had twice as many snapshots as we needed, and also
defined twice as many edge addresses as we needed.
Test Plan:
Note that snapshot contents are either deleted or unchanged.
wchargin-branch: simplify-github-edge-tests
Test Plan:
Observe that the new snapshots are easier to read. Might as well make
sure that they encode the same data as the old snapshots, too. Note that
the backslash character no longer appears in this snapshot file. :-)
wchargin-branch: use-edgeToParts
Summary:
We have `edgeToString`, which formats edges as nicely human-readable
strings. However, these strings have some quotes in them, and so when
they are themselves stringified (e.g., as part of a Jest snapshot), they
become much harder to read. We thus introduce `edgeToParts` to make our
snapshots more readable.
Test Plan:
Unit tests added; run `yarn travis`.
wchargin-branch: add-edgeToParts
Summary:
In #394, we uppercased the constants for GitHub node types. However, we
were using string literals instead of constants in the test cases. These
test cases were supposed to cover every error path, but instead ended up
just covering the “bad type” error path many times.
Any one of the following would have prevented this regression:
1. using string constants instead of literals in the test case;
2. throwing and checking more precise error messages; or
3. being alerted that coverage decreased as a result of the change.
In this commit, we enact the first of these options. I’m open to adding
a coverage bot, but don’t feel strongly about it at this time.
Test Plan:
Running `yarn coverage` now shows 100% coverage for the `nodes.js`
module, whereas previously almost all `throw fail();` lines were
uncovered (and the branch coverage was just 76%).
wchargin-branch: fix-github-node-error-tests
- Switch string constant node and edge types (e.g. "REPO") to exported
consts (eg `export const REPO_TYPE`).
- Add (and internally use) a `_Prefix` psuedomodule which contains
per-type address prefixes
- Test that constructing a StructuredAddress with the wrong type is an
error.
Test plan:
Unit tests pass, snapshots unchanged.
Paired with @wchargin
Summary:
This fixes up all instances of `fromParts([])` that are not in
`address.js` or `address.test.js`.
Paired with @decentralion.
Test Plan:
Running `git grep --name-only -F 'fromParts([])'` yields only the two
modules listed above. Existing unit tests suffice for correctness.
wchargin-branch: use-address-empty
Summary:
This can make invocations of `FooAddress.fromParts([])` a bit more
succinct.
Paired with @decentralion.
Test Plan:
Unit tests added. Run `yarn travis`.
wchargin-branch: address-empty
Similar to #390, we now allow filtering the results from `Graph.edges`
by address prefixes. It's a little more complicated than #390, as we
allow filtering by src, dst, or address.
Test plan:
Unit tests added. `yarn travis` passes.
Paired with @wchargin
Simple API addition to match v1/v2 semantics.
In the future, we can perf optimize this if we switch graph to
store nodes organized by shared prefixes.
Test plan:
Unit tests were added. `yarn travis` passes.
Paired with @wchargin
Summary:
This module includes a raw edge type, a structured edge type, and edge
creation functions that take source and destination and create an edge.
Test Plan:
Unit tests added. These cover all of the successful cases, and none of
the unsuccessful cases. We plan to refactor this code Soon™, and it is
hard to see how to nicely factor the tests without just testing the same
code paths over and over.
wchargin-branch: github-edges
Summary:
First, we rename the module itself from `address` to `nodes`: we’d like
to put the edge functions in a parallel `edges` module instead of
cramping it into this one, so it stands to reason that this one should
be called `nodes`.
We also rename the `GithubAddressT` type to `RawAddress`, so that the
module exports `RawAddress` and `StructuredAddress`. The functions then
have much better natural names of `toRaw` and `fromRaw`.
Test Plan:
Existing unit tests suffice.
wchargin-branch: rename-nodes
One of the slight modifications we've made in v3 is to effect the
following renames (as implemented in #380):
PullRequest -> Pull
PullRequestReview -> Review
PullRequestReviewComment -> ReviewComment
This commit just changes the rest of the github code in v3 to follow the
new convention.
Test plan:
`yarn travis --full` passes.
Summary:
This has two primary benefits:
- Humans can look at this snapshot file to see what’s being queried,
or to manually issue a query.
- When we change the programmatically generated query, we can easily
see what the results are in the GraphQL output. This makes it easy
to verify that a change is correct.
Test Plan:
None.
wchargin-branch: snapshot-query
Summary:
This module exposes a structured type `StructuredAddress`, an embedding
`GithubAddressT` of this type into the `NodeAddress` layer, and
functions to convert between the two.
Paired with @wchargin.
Test Plan:
Unit tests added, with full coverage. Snapshots are easily readable.
This commit copies the following logic necessary for downloading GitHub
data into v3. Minimal changes have been made to accomodate the new path
structure.
Test plan:
- Manually ran plugins/github/fetchGithubRepoTest.sh and verified that
it can correctly pass and fail
- Added the v3 github repo test to `yarn travis --full`
- Ran `yarn travis --full` and it passed
Paired with @wchargin
Summary:
We’ve added a comment directly on a pull request.
Paired with @decentralion.
Test Plan:
`yarn travis --full` passes.
wchargin-branch: update-example-github
Summary:
Each of the invariants listed at the top of the `Graph` class is now
explicitly checked by `checkInvariants`, which is called at the end of
each `Graph` method during tests only. This is powerful: it means that
not only do our tests for `Graph` test the graph, but also any tests
that depend on `Graph`—e.g., plugin code—will give us extra invariant
testing on `Graph`. As noted in a comment, if this becomes bad for
performance, we can blacklist expensive tests or whitelist tests that we
care about.
A graph method may assume that the graph invariants hold before the
method is invoked. Within the body of a graph method, invariants may be
violated, but the method must ensure that the invariants hold
immediately before it returns or yields. A consequence of this is that
if a graph function internally calls a public function (e.g., `addEdge`
might call `hasNode` to check that the source and destination exist),
then it must ensure that the invariants hold before the internal call.
This is not an “implementation detail” or “caveat”; it is simply part of
the interface of public functions. It is legal and reasonable for
private helper functions to explicitly not expect or not guarantee that
particular invariants hold, and in this case the exception should be
documented. (This is not yet the case in any of our code.)
Finally, note that the `checkInvariants` method should not call any
public methods, because those methods in turn call `checkInvariants`. If
this becomes a huge pain, we can look into implementing some kind of
“only check invariants if the invariants are not actively being
checked”, but I’d much rather not do so if we don’t have to.
Test Plan:
Running `yarn coverage` indicates that each of the failure cases is
verified. In principle, I’d be willing to add a test that parses the
source code for `graph.js` and verifies that each `return`, `yield`, or
implicit return is preceded by an invariant check. But I don’t really
want to implement that right now.
wchargin-branch: automatic-invariants
The `advancedGraph` is an example graph defined in `graph.test.js`.
It shows off many tricksy features, like having loop edges, multiple
edges from the same src to same dst, etc. We also provide two ways of
constructing it: `graph1` is straightforward, `graph2` adds tons of
spurious adds, removes, and odd ordering. This way we can ensure that
our functions treat `graph1` and `graph2` equivalently.
Test plan:
New unit tests are added verifying that `equals`, `merge`, and
`to/fromJSON` handle the advanced graph appropriately.
The serialization scheme uses `IndexedEdge`s:
```js
type Integer = number;
type IndexedEdge = {|
Address: EdgeAddressT,
srcIndex: Integer,
dstIndex: Integer,
|}
```
The nodes are first sorted. Then, we generate indexed edges from the
regular edges by replacing each node address with its index in the
sorted order. This encoding reduces the number of addresses serialized
from `n + 3e` to `n + e` (where `n` is the number of nodes and `e` is
the number of edges).
This is based on work in #295, but in contrast to that PR, we do not
index the in-memory representations of graphs. Only the JSON
representation is indexed.
Test plan:
Unit tests added. A snapshot test is also included, both to make it easy
to inspect an example of a JSON-serialized graph, and to ensure
backwards-compatibility. (The snapshot likely should not change
independent of the VERSION string.)
* Implement `Graph.merge`
Tests are mostly copied over from the v2, as implemented in #320.
Some new tests were added, e.g. checking that Merge correctly handles
10 small graphs combined.
Test plan:
See unit tests.
* Reimplement `Graph.copy` using `Graph.merge`
Test plan:
Existing unit tests suffice
Suggested by @wchargin
Tests are mostly copied over from the v2, as implemented in #320.
Some new tests were added, e.g. checking that Merge correctly handles
10 small graphs combined.
Test plan:
See unit tests.
Summary:
The previously listed invariants were weak on two counts. First, it was
unstated that the keys of `_inEdges`, `_outEdges`, and `_nodes` should
coincide. Second, the “exactly once” condition on edge inclusion had the
unintentional effect that edge absent in `_edges` but present twice or
more in each of `_inEdges` and `_outEdges` would not violate the
invariant.
Test Plan:
Stay tuned.
wchargin-branch: strengthen-invariants
The implementation is quite simple. The tests are somewhat more
comprehensive than in v2 or v1. We now test that copies are equal to the
original in a variety of situations.
Test plan:
Unit tests added.
It turns out we forgot to add this to the API, so I added it. I also
implemented it. The tests are pretty thorough; as an added innovation
over our previous tests (e.g. in #312 and #61), we now consistently test
that equality is commutative.
In contrast to our previous implementations, this one is massively
simpler. That's an upside of using primitive ES6 data structures to
store all of the graph's information... which is itself an upside of not
trying to store arbitrary additional information in the graph. Now we
can just do a deep equality check on the underlying nodes set and edges
map!
We might be able to performance tune this method by taking advantage of
the structure of our nodes and edges. This should suffice for now,
though.
Paired with @wchargin
Test plan:
Unit tests were added. Run `yarn travis`
This is the change that puts the Graph into `Graph` :) We add `_inEdges`
and `_outEdges`, and use them to identify the neighbors of a given
`node`.
The API is implemented pretty uncontroversially. (We've done this a few
times before: see #319, #162). As with other iterators, we check for
comodification and error if this has occurred.
The tests cover some interesting cases like absent nodes, loops, and
multiple edges with the same src and dst.
Test plan:
Unit tests have been added. Run `yarn travis`.
Paired with: @wchargin
Summary:
A client of `Graph` is able to (e.g.) invoke `nodes()` to get a node
iterator, iterate over some of the nodes, then change the nodes by
adding or removing the nodes on the original graph. The semantics of
what to do here are not clear: ES6 specifies semantics for `Map` and
`Set`, but they have counterintuitive consequences: for instance, you
can get a `Set` iterator to yield the same value twice. Most collection
implementations in the Java standard library prohibit this entirely. In
this commit, we adopt the latter approach.
A caveat of this implementation is that a graph object may not be
mutated more than 2^53 − 1 (`Number.MAX_SAFE_INTEGER`) times. Clients
who need to mutate a graph more than nine quadrillion times are
encouraged to reconsider their data model.
Paired with @decentralion.
Test Plan:
Unit tests added. Run `yarn travis`.
wchargin-branch: comod-check
Adding a conflicting edge (i.e. one with the same address, but different
`src` or `dst`) is an error. However, our coverage has determined that
the behavior isn't tested. This commit adds that test.
Test plan:
Only change is adding a new test. Verify `yarn travis` passes.
Summary:
We’re not mandating anything about coverage right now, but by making it
easier to track coverage perhaps people will organically become more
motivated to write good tests.
Test Plan:
Run `yarn coverage`, and then open `coverage/lcov-report/index.html`.
wchargin-branch: coverage
Test Plan:
Run `yarn test --env=jsdom --coverage`, and note that files are
generated into `coverage/`. Then, run `yarn travis`, which fails before
this patch (on `check-pretty`) and passes after it.
wchargin-branch: prettier-ignore-coverage
Summary:
These are superseded by the unified implementation in `address.js`.
Test Plan:
Existing unit tests suffice.
wchargin-branch: remove-legacy-address
Summary:
This is basically a textual substitution.
Test Plan:
Existing unit tests suffice. Note that `_address.js` (with underscore)
is no longer imported except from its own tests.
wchargin-branch: unified-addresses-in-graph
Summary:
This implements the following functions for the unified addresses:
- assertions: `assertValid`, `assertValidParts`
- injection: `fromParts`
- projection: `toParts`
(These functions depend on each other for testing, so we implement them
together.)
Test Plan:
Unit tests included. Run `yarn travis`.
wchargin-branch: address-assertion-injection-projection
William Chargin
Dandelion Mané