See #1243 for context. This is basically a more aggressive version of
pull #1230 -- instead of just running unit tests in isolation, we also
run flow in isolation, and kill the servers afterwards.
Test plan: See how this fares in CI :)
Ever since I upgraded all of the dependencies, we've been having
regular CI failures, which seem to share a common root cause of memory
exhaustion. Here are some examples: [1], [2].
[1]: https://circleci.com/gh/sourcecred/sourcecred/1246
[2]: https://circleci.com/gh/sourcecred/sourcecred/1239
After some experimentation, I've found that we can solve the
issue by ensuring that jest runs on its own in CI, so that it doesn't
contend with other tests for memories. Also, I reduce its max workers to
2, which matches the number of CPUs in the CircleCI containers.
Unfortunately, this does increase our build time. The postcommit (non
full) test now takes 45-60s (up from 30-50s), and the full test is also
a little slower. However, building in about one minute is still
acceptably fast, and having regular flakey test failures is not
acceptable, so this is still a win.
If we want to improve on this in the future, we should look into the git
shells getting spawned in `config/env.js`. I noticed that they were
often involved in the out-of-memory failures.
Also, I modified `.circleci/config.yml` so that any branch matching the
regular expression `/ci-.*/` will trigger a full build. That makes it
easier to test against CI failures.
Test plan: I ran about ~10 full builds with this change, and more with
similar variations, and they all passed. Verify that the full builds
that are run for this commit also all pass! Also, verify that running
yarn test locally has unchanged behavior, and running
`yarn test --ci` locally lets jest run to completion before running
any other test.
This commit adds a module, `fetchGithubOrg`, which loads data on GitHub
organizations, most notably including the list of repositories in that
org.
The structure of this commit is heavily influenced by review feedback
from @wchargin's [review] of a related PR.
Test plan: This logic depends on actually hitting GitHub's API, so the
tests are modeled off the related `fetchGithubRepo` module. There is a
new shell test, `src/plugins/github/fetchGithubOrgTest.sh`, which
verifies that that the org loading logic works via a snapshot.
To verify the correctness of this commit, I've performed the following
checks:
- `yarn test --full` passes
- inspection of `src/plugins/github/example/example-organization.json`
confirms that the list of repositories matches the repos for the
"sourcecred-test-organization" organization
- manually breaking the snapshot (by removing a repo from the snapshot)
causes `yarn test --full` to fail
- running `src/plugins/github/fetchGithubOrgTest.sh -u` restores the
snapshot, afterwhich `yarn test --full` passes again.
[review]: https://github.com/sourcecred/sourcecred/pull/1089#pullrequestreview-204577637
Currently, our underlying test script uses npm rather than yarn to
execute the tests. This is awkward, because we use yarn everywhere else
in lieu of npm. It turns out that some setups have node available
without npm, and in such environments our tests fail with a cryptic
ENOENT error.
This changes the tests to use yarn instead.
Test plan: `yarn test --full` passes.
Thanks to @wpank for help uncovering this issue.
This commit adds a new runOption for execDependencyGraph, namely
`printVerboseResults`. If this flag is true, then execDependencyGraph
will print a "Full Results" section along with the standard error and
standard out of every task, regardless of whether it failed or
succeeded. (Note, this is the existing behavior for all invocations
prior to this commit).
If the flag is not true, then execDependencyGraph will not print a full
results section, and stdout/stderr will be logged only for tasks that
fail.
This commit also modifies `yarn test` to use the new flag so that it
prints verbose tests only when the `--full` option is provided. This is
consistent with our sharness behavior: we print the full sharness logs
only when `--full` was provided.
This fixes#1035, and ensures that running `yarn test` has a high signal
to noise ratio (i.e. it only shows an enumeration of top level tasks).
This improves the developer ergonomics of SourceCred by not having a
super commonly used and core script spam the user with mostly irrelevant
information.
Test plan:
Run `yarn test` when all tests are passing, and observe that the output
has much less noise:
```
yarn run v1.12.3
$ node ./config/test.js
tmpdir for backend output: /tmp/sourcecred-test-6337SZ9smvWsWvqE
Starting tasks
GO ensure-flow-typing
GO check-stopships
GO check-pretty
GO lint
GO flow
GO unit
GO backend
PASS check-stopships
PASS ensure-flow-typing
PASS flow
PASS backend
GO sharness
PASS sharness
PASS check-pretty
PASS lint
PASS unit
Overview
Final result: SUCCESS
Done in 11.66s.
```
Run `yarn test` when there is a real failure (e.g. a unit test failure)
and observe that full details on the failure, including the output from
stdout/stderr, is still provided.
Run `yarn test --full` and observe that full, verbose logs are provided.
As described in #1033: Currently, developers in environments without gnu
coreutils (notably: macOS) get an extremely confusing error message when
they run `yarn test`. This commit ensures that such users instead get a
helpful message with a link to the fix.
Follows @wchargin's proposed approach for fixing #1033, so that Mac
developers get clear guidance on how to get their development
environment working properly.
Expected behavior is that `yarn test` passes when GNU coreutils are
present, and fails with the following message when they are not:
Fixes#1033
```
FAIL check-gnu-coreutils
Exit code: 1
Contents of stdout:
/home/dandelion/git/sc/sourcecred
Contents of stderr:
Error: Your environment does not provide GNU coreutils
You're likely developing on macOS.
Please see the following link for a fix:
https://github.com/sourcecred/sourcecred/issues/698#issuecomment-417202213
```
Test plan:
Verify that on a machine with gnu coreutils present, the command passes
and does not print irrelevant output.
Verify that on a machine without gnu coreutils available, the command
fails, and prints exactly the error message expected, without any extra
output related to the test's implementation.
Paired-with: @anthrocypher
Summary:
Prior to this commit, running `yarn test full` (instead of `--full`)
would silently behave the same way as `yarn test`. This is misleading.
It now fails.
Test Plan:
Run `! yarn test full && yarn test --full`.
wchargin-branch: test-validate-arguments
Summary:
Closes#902. CircleCI continues to work fast and well, on both the
commit and nightly workflows. Travis continues to be slower. It is time.
I have disabled Travis for `sourcecred/sourcecred` via:
<https://travis-ci.org/profile/sourcecred>
This commit removes the vestigial configuration files and code.
Test Plan:
Running `yarn test` still works. Running `yarn test --full` still works,
and properly invokes `sharness-full` and the various extra tests.
Running `git grep -i travis` yields no results.
wchargin-branch: ci-remove-travis
According to the [jest docs], setting `maxWorkers=4` can substantially
improve performance on CI.
This commit sets `maxWorkers=4` when running `yarn unit` as a part of
`yarn test`. Based on my local testing (see data below), this improves
performance locally in addition to the expected performance improvement
on travis.
\## Testing `yarn unit` by itself
```
yarn unit --maxWorkers=1
15.28s user 3.71s system 112% cpu 16.848 total
15.13s user 3.68s system 112% cpu 16.708 total
15.32s user 3.76s system 112% cpu 16.917 total
15.91s user 3.74s system 112% cpu 17.425 total
15.61s user 3.76s system 113% cpu 17.125 total
yarn unit --maxWorkers=2
19.43s user 4.03s system 212% cpu 11.061 total
19.86s user 4.19s system 210% cpu 11.407 total
21.19s user 4.26s system 213% cpu 11.902 total
20.68s user 4.51s system 212% cpu 11.873 total
20.78s user 4.26s system 212% cpu 11.780 total
yarn unit --maxWorkers=4
29.43s user 5.14s system 389% cpu 8.865 total
29.99s user 5.18s system 392% cpu 8.961 total
32.22s user 5.52s system 390% cpu 9.659 total
33.99s user 5.97s system 395% cpu 10.097 total
33.38s user 5.93s system 395% cpu 9.933 total
yarn unit --maxWorkers=8
48.21s user 6.57s system 621% cpu 8.815 total
51.61s user 7.16s system 610% cpu 9.622 total
59.48s user 7.82s system 621% cpu 10.833 total
58.18s user 8.10s system 624% cpu 10.607 total
58.92s user 8.22s system 620% cpu 10.817 total
unset
46.27s user 6.44s system 599% cpu 8.799 total
49.08s user 7.04s system 600% cpu 9.342 total
54.85s user 7.52s system 600% cpu 10.383 total
55.66s user 7.52s system 605% cpu 10.438 total
53.77s user 7.50s system 604% cpu 10.142 total
```
\## Testing `yarn test`
```
maxWorkers=1
46.65s user 5.92s system 249% cpu 21.038 total
47.94s user 5.81s system 251% cpu 21.354 total
51.50s user 6.44s system 260% cpu 22.234 total
52.60s user 6.65s system 268% cpu 22.077 total
53.04s user 6.27s system 266% cpu 22.278 total
maxWorkers=2
56.13s user 6.13s system 409% cpu 15.204 total
63.32s user 7.22s system 412% cpu 17.091 total
64.82s user 7.19s system 422% cpu 17.027 total
64.59s user 7.41s system 417% cpu 17.227 total
65.40s user 7.30s system 419% cpu 17.318 total
maxWorkers=4
74.64s user 7.60s system 584% cpu 14.066 total
82.69s user 8.43s system 582% cpu 15.643 total
85.00s user 8.68s system 591% cpu 15.835 total
84.81s user 8.58s system 595% cpu 15.690 total
85.22s user 8.59s system 596% cpu 15.719 total
maxWorkers=4 and everything depends on unit
59.29s user 6.01s system 378% cpu 17.261 total
62.99s user 6.64s system 375% cpu 18.564 total
65.54s user 7.31s system 375% cpu 19.419 total
63.24s user 7.13s system 379% cpu 18.548 total
63.68s user 7.13s system 383% cpu 18.457 total
maxWorkers=8
92.85s user 8.13s system 643% cpu 15.702 total
101.63s user 9.21s system 632% cpu 17.510 total
101.63s user 9.23s system 636% cpu 17.428 total
101.81s user 9.32s system 633% cpu 17.546 total
101.62s user 9.39s system 632% cpu 17.542 total
unset
88.75s user 8.15s system 646% cpu 14.988 total
96.43s user 9.23s system 631% cpu 16.739 total
98.27s user 9.17s system 638% cpu 16.819 total
98.46s user 9.01s system 642% cpu 16.729 total
98.53s user 9.15s system 637% cpu 16.889 total
unset + everything depends on unit
76.02s user 7.61s system 486% cpu 17.208 total
79.14s user 8.26s system 484% cpu 18.030 total
84.32s user 9.19s system 488% cpu 19.136 total
84.92s user 9.14s system 497% cpu 18.919 total
84.46s user 8.94s system 492% cpu 18.965 total
```
Test plan: `yarn test` passes here and on travis
[jest docs]: https://jestjs.io/docs/en/troubleshooting
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
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
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
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
Summary:
We will shortly want to perform testing of shell scripts; it makes the
most sense to do so via the shell. We could roll our own testing
framework, but it makes more sense to use an existing one. By choosing
Sharness, we’re in good company: `go-ipfs` and `go-multihash` use it as
well, and it’s derived from Git’s testing library. I like it a lot.
For now, we need a dummy test file; our test runner will fail if there
are no tests to run. As soon as we have a real test, we can remove this.
This commit was generated by following the “per-project installation”
instructions at https://github.com/chriscool/sharness, and by
additionally including that repository’s `COPYING` file as
`SHARNESS_LICENSE`, with a header prepended. I considered instead adding
Sharness as a submodule, which is supported and has clear advantages
(e.g., you can update the thing), but opted to avoid the complexity of
submodules for now.
Test Plan:
Create the following tests in the `sharness` directory:
```shell
$ cat sharness/good.t
#!/bin/sh
test_description='demo of passing tests'
. ./sharness.sh
test_expect_success "look at me go" true
test_expect_success EXPENSIVE "this may take a while" 'sleep 2'
test_done
# vim: ft=sh
$ cat sharness/bad.t
#!/bin/sh
test_description='demo of failing tests'
. ./sharness.sh
test_expect_success "I don't feel so good" false
test_done
# vim: ft=sh
```
Note that `yarn sharness` and `yarn test` fail appropriately. Note that
`yarn sharness-full` fails appropriately after taking two extra seconds,
and `yarn test --full` runs the latter. Each failure message should
print the name of the failing test case, not just the suite name, and
should indicate that the passing tests passed.
Then, remove `sharness/bad.t`, and note that the above commands all
pass, with the `--full` variants still taking longer.
Finally, remove `sharness/good.t`, and note that the above commands all
pass (and all pass quickly).
wchargin-branch: add-sharness
Summary:
Running `yarn test` (equiv. `npm test` or `npm run test`) now runs all
checks. It takes the place of the former `yarn travis`. This is more in
line with the expectation of a top-level `test` command: if it passes,
your code is good.
The `unit` command now runs Jest once, not in watch mode. It takes the
place of the former `ci-test`. To run tests in watch mode, run any of
the following:
- `yarn unit --watch`, or
- `npm run unit -- --watch`, or
- `npm unit -- --watch`.
This behavior is more consistent with the standard behavior of commands
like `make test`. It is also empirically what @wchargin and
@decentralion want most of the time.
Test Plan:
Verify that each of the scripts `test`, `unit`, and `coverage` passes.
Verify that each of the aforementioned `--watch` invocations works.
Verify that `.travis.yml` has the correct `script:` command.
wchargin-branch: reorganize-test-command
Dandelion Mané
William Chargin