Add end-to-end tests #33
Merged
+2,024
−13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Set up a Cucumber (TypeScript) test framework in the 'test/e2e' directory. This commit contains only the infrastructure needed to write and run the tests; the tests themselves will be added in future commits. Signed-off-by: Victoria Dye <vdye@github.com>
Add 'basic.feature' to test common workflows using the bundle server. Initially, add a common "Background" step starting the bundle web server at the specified port, and a common "After" step run when a test ends (successful or not) stopping that web server process. Signed-off-by: Victoria Dye <vdye@github.com>
derrickstolee
approved these changes
Mar 17, 2023
Comment on lines
+6
to
+11
| Scenario: A user can clone with a bundle URI pointing to the bundle server | ||
| Given a remote repository 'https://github.com/vdye/asset-hash.git' | ||
| Given the bundle server has been initialized with the remote repo | ||
| When I clone from the remote repo with a bundle URI | ||
| Then bundles are downloaded and used |
There was a problem hiding this comment.
This is an interesting formal mechanism for describing tests. Each of these lines are repeatable steps that can be mixed and matched. For instance, I could imagine (not looking ahead) a test like:
Scenario: User fails to clone from a non-existing bundle URI
Given a remote repository 'https://github.com/vdye/asset-hash.git'
When I clone from the remote repo with a bundle URI
Then bundles are not downloaded and clone succeeds with a warning
The only new step to build is the Then statement. Neat!
Comment on lines
+22
to
+72
| // Ensure warning wasn't thrown | ||
| clonedRepo.cloneResult.stderr.toString().split("\n").forEach(function (line) { | ||
| if (line.startsWith("warning: failed to download bundle from URI")) { | ||
| assert.fail(line) | ||
| } |
There was a problem hiding this comment.
I wonder if it would be worth asking git for-each-ref --format="%(refname)" "refs/bundles/*" if it has at least one ref?
Comment on lines
35
to
40
| result = clonedRepo.runGit("for-each-ref", "--format=%(refname)") | ||
| utils.assertStatus(0, result, "git for-each-ref failed") | ||
|
|
||
| const bundleRefRegex = new RegExp('^refs/bundles/.*$') | ||
| const bundleRefs = result.stdout.toString().split("\n").filter(function (line) { | ||
| return bundleRefRegex.test(line) |
There was a problem hiding this comment.
Ah, you do it here! We could add the refs/bundles/* filter to the command, then we don't need to regex on the result, just look for at least one line.
Comment on lines
+12
to
+43
| Scenario: A user can fetch with a bundle server that's behind and get all updates | ||
| Given a new remote repository with main branch 'main' | ||
| Given another user pushed 10 commits to 'main' | ||
| Given the bundle server has been initialized with the remote repo | ||
| Given I cloned from the remote repo with a bundle URI | ||
| Given another user pushed 2 commits to 'main' | ||
| When I fetch from the remote | ||
| Then I am up-to-date with 'main' | ||
| Then my repo's bundles are not up-to-date with 'main' | ||
|
|
||
| Scenario: A user will fetch incremental bundles to stay up-to-date | ||
| Given a new remote repository with main branch 'main' | ||
| Given another user pushed 10 commits to 'main' | ||
| Given the bundle server has been initialized with the remote repo | ||
| Given I cloned from the remote repo with a bundle URI | ||
| Given another user pushed 2 commits to 'main' | ||
| Given the bundle server was updated for the remote repo | ||
| When I fetch from the remote | ||
| Then I am up-to-date with 'main' | ||
| Then my repo's bundles are up-to-date with 'main' | ||
|
|
||
| Scenario: A user can fetch force-pushed refs from the bundle server | ||
| Given a new remote repository with main branch 'main' | ||
| Given another user pushed 10 commits to 'main' | ||
| Given the bundle server has been initialized with the remote repo | ||
| Given I cloned from the remote repo with a bundle URI | ||
| Given another user removed 2 commits and added 4 commits to 'main' | ||
| Given the bundle server was updated for the remote repo | ||
| When I fetch from the remote | ||
| Then I am up-to-date with 'main' | ||
| Then my repo's bundles are up-to-date with 'main' |
There was a problem hiding this comment.
😍 this is amazing to read and clearly understand the scenarios.
test/e2e/features/support/world.ts
Outdated
|
|
||
| const clonedRepo = this.getRepo(user) | ||
|
|
||
| // TODO: figure out a better way to check whether the repo is empty |
There was a problem hiding this comment.
git rev-list --all -n 1 has no output?
Makefile
Outdated
Comment on lines
46
to
50
| e2e-test: build | ||
| @echo | ||
| @echo "======== Running end-to-end tests ========" | ||
| $(RM) -r $(TESTDIR) | ||
| @if [ -n "$(TEST_E2E_PERF)" ]; then \ |
There was a problem hiding this comment.
Looks like make e2e-test and make e2e-test TEST_E2E_PERF=1 will run the tests and performance tests, respectively.
Could you update the README with these test instructions?
There was a problem hiding this comment.
I also had to update my version of node.js to work with cucumber. It was really old, but maybe that should be listed as a dependency for developers?
Comment on lines
+3
to
+5
| on: | ||
| pull_request: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
As you mentioned, "only on PRs" is quite nice because that way it's not on arbitrary pushes and not re-testing after a PR merged (and was subject to required builds).
The manual trigger of the workflow (with optional perf tests!) is excellent.
Comment on lines
14
to
17
| Examples: | ||
| | repo | | ||
| | https://github.com/git/git.git | # takes ~2 minutes | ||
| | https://github.com/torvalds/linux.git | # takes ~30(!) minutes |
There was a problem hiding this comment.
It took me a while to realize that this literally supplies the input to the tests. Awesome.
There was a problem hiding this comment.
Here are my test results for the data you have so far:
Clone execution time for https://github.com/git/git.git: 11.64416905105114s (bundle URI) vs. 22.594083791017532s (no bundle URI)
Clone execution time for https://github.com/torvalds/linux.git: 168.95288394904136s (bundle URI) vs. 302.48135003399847s (no bundle URI)
Here are some others I tested:
Clone execution time for https://github.com/git-for-windows/git.git: 14.297109727025033s (bundle URI) vs. 29.787424183011055s (no bundle URI)
Clone execution time for https://github.com/kubernetes/kubernetes.git: 37.84783263194561s (bundle URI) vs. 41.304983952999116s (no bundle URI)
Clone execution time for https://github.com/homebrew/homebrew-core: 31.01036406993866s (bundle URI) vs. 357.1608746279478s (no bundle URI)
If you want to include any of them, here is my example list (minus git-for-windows/git since it's not materially different from git/git):
Examples:
| repo |
| https://github.com/git/git.git | # takes ~2 minutes
| https://github.com/kubernetes/kubernetes.git | # takes ~3 minutes
| https://github.com/homebrew/homebrew-core | # takes ~15(!) minutes
| https://github.com/torvalds/linux.git | # takes ~30(!) minutes
There was a problem hiding this comment.
Thanks, I'll add those cases!
Seeing your results, my timing comments could be misleading - there will be a ton of variability in execution time depending on internet speed (primarily), system resources, etc. I'll remove them and add a disclaimer in README.md along the lines of "The performance tests clone very large repos and can take anywhere from ~30 minutes to multiple hours to run, depending on internet connectivity and other system resources".
On that note, I'll also add an @online tag and corresponding "offline" cucumber.js profile to allow people to run tests without internet.
ldennington
approved these changes
Mar 17, 2023
| @@ -0,0 +1,7 @@ | |||
| import { defineParameterType } from '@cucumber/cucumber' | |||
|
|
|||
| defineParameterType({ | |||
There was a problem hiding this comment.
You've found a lot of cool ways to integrate useful Cucumber features - it's been really helpful for ramping!
|
I played around with extending your tests, so I could get a feel of how it works. The result is de09244, which let me repeat the performance tests, but used a second machine with existing bundles instead of creating new ones from scratch. (Perhaps these should be the same, with the environment variable switching modes at runtime instead of failing without it.) |
Add a test and its corresponding step definitions verifying that a remote repository initialized in a bundle server can be cloned with '--bundle-uri' to download bundles. Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Victoria Dye <vdye@github.com>
Add scenarios to the end-to-end tests involving updates to the remote and/or bundle server. As part of this update, add a custom Cucumber expression for matching "are/are not" to a boolean. Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Victoria Dye <vdye@github.com>
Add a test that captures timing measurements (in 'console.log()' printouts) for cloning various large repositories. The tests are divided into two example sets: one for tests that run on modern hardware with a good internet connection for a *total* of less than 15 minutes (currently take ~7 minutes), and the other for tests that take longer than that. The latter set is tagged with '@slow' and the 'default' profile is updated to skip them. Add an 'all' profile that runs these slow performance tests alongside the "regular" tests. Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Victoria Dye <vdye@github.com>
Tag tests that require internet access with '@online' and add a Cucumber profile that skips those tests. This allows developers to easily run tests on the bundle server in an offline or bandwidth-limited environment. Signed-off-by: Victoria Dye <vdye@github.com>
Update the workflows that build the project to use the fixed version of Go tied to the project in 'go.mod'. The 'go.mod' can be used as the sole source of truth on the supported version of Go in the repository, making it easier to upgrade and use new features when needed. Signed-off-by: Victoria Dye <vdye@github.com>
Add a target to the Makefile that installs the end-to-end test NPM dependencies then runs the tests. By default, this will only run tests matching the "default" profile of the tests (excluding performance tests). The 'E2E_FLAGS' variable can be used to specify the profile(s) used by the tests (e.g., '--all' to include the tests excluded by default). Add documentation on how to run the end-to-end tests to 'README.md'. Signed-off-by: Victoria Dye <vdye@github.com>
Add a dedicated 'test.yml' workflow that executes the end-to-end tests. Unlike CI, these tests are only run automatically for pull requests (although they can be manually triggered via workflow dispatch). By default, the tests will run with the "default" profile; this can be changed via an input variable in the workflow dispatch. Because the end-to-end tests deal with external dependencies (namely, Git), add an 'install-dependencies.sh' script to install and configure the dependencies on a GitHub Actions runner. The goal of this workflow is for it to eventually be used to run multiple types of long-running tests, such as integration tests or installer validation tests. Signed-off-by: Victoria Dye <vdye@github.com>
|
I ended up making more substantial changes than I initially intended. 😅 Beyond the suggestions in @derrickstolee's review:
If anyone's interested, here's the range-diff! Range diff1: a817a95 ! 1: e8a38b0 e2e: add initial bundle server clone test
@@ Commit message
repository initialized in a bundle server can be cloned with '--bundle-uri'
to download bundles.
+ Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
## .gitignore ##
@@ test/e2e/features/step_definitions/repository.ts (new)
+ const actualURI = result.stdout.toString().trim()
+ assert.strictEqual(actualURI, this.bundleServer.bundleUri())
+
-+ result = clonedRepo.runGit("for-each-ref", "--format=%(refname)")
++ result = clonedRepo.runGit("for-each-ref", "--format=%(refname)", "refs/bundles/*")
+ utils.assertStatus(0, result, "git for-each-ref failed")
+
-+ const bundleRefRegex = new RegExp('^refs/bundles/.*$')
-+ const bundleRefs = result.stdout.toString().split("\n").filter(function (line) {
-+ return bundleRefRegex.test(line)
++ const bundleRefs = result.stdout.toString().split("\n").filter(function(line) {
++ return line.trim() != ""
+ })
+ assert.strict(bundleRefs.length > 0, "No bundle refs found in the repo")
+})
2: 99d8db1 ! 2: 368fa56 e2e: add tests updating the remote & bundle server
@@ Commit message
bundle server. As part of this update, add a custom Cucumber expression for
matching "are/are not" to a boolean.
+ Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
## test/e2e/cucumber.js ##
@@ test/e2e/features/support/world.ts: export class BundleServerWorld extends World
+
+ const clonedRepo = this.getRepo(user)
+
-+ // TODO: figure out a better way to check whether the repo is empty
-+ if (clonedRepo.runGit("show").status != 0) {
++ const result = clonedRepo.runGit("rev-list", "--all", "-n", "1")
++ if (result.stdout.toString().trim() == "") {
+ // Repo is empty, so make sure we're on the right branch
+ utils.assertStatus(0, clonedRepo.runGit("branch", "-m", branch))
+ } else {
3: 40dd79c ! 3: c9ee08f e2e: add performance test
@@ Commit message
e2e: add performance test
Add a test that captures timing measurements (in 'console.log()' printouts)
- for cloning various large repositories. Because these tests can be
- exceptionally slow, tag them with '@slow' and make sure they are not run in
- the 'default' profile. Add an 'all' profile that runs the performance tests
- alongside the "regular" tests.
+ for cloning various large repositories. The tests are divided into two
+ example sets: one for tests that run on modern hardware with a good internet
+ connection for a *total* of less than 15 minutes (currently take ~7
+ minutes), and the other for tests that take longer than that. The latter set
+ is tagged with '@slow' and the 'default' profile is updated to skip them.
+ Add an 'all' profile that runs these slow performance tests alongside the
+ "regular" tests.
+ Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
## test/e2e/cucumber.js ##
@@ test/e2e/features/performance.feature (new)
+ Background: The bundle web server is running
+ Given the bundle web server was started at port 8080
+
-+ @slow
+ Scenario Outline: Comparing clone performance
+ Given a remote repository '<repo>'
+ Given the bundle server has been initialized with the remote repo
@@ test/e2e/features/performance.feature (new)
+ Then I compare the clone execution times
+
+ Examples:
-+ | repo |
-+ | https://github.com/git/git.git | # takes ~2 minutes
-+ | https://github.com/torvalds/linux.git | # takes ~30(!) minutes
++ | repo |
++ | https://github.com/git/git.git |
++ | https://github.com/kubernetes/kubernetes.git |
++
++ @slow
++ Examples:
++ | repo |
++ | https://github.com/homebrew/homebrew-core |
++ | https://github.com/torvalds/linux.git |
## test/e2e/features/step_definitions/repository.ts ##
@@ test/e2e/features/step_definitions/repository.ts: When('I clone from the remote repo with a bundle URI', async function (this: Bun
@@ test/e2e/features/step_definitions/repository.ts: Then('my repo\'s bundles {bool
+ utils.assertStatus(0, myClone.cloneResult)
+ utils.assertStatus(0, otherClone.cloneResult)
+
-+ console.log(`\nClone execution time for ${this.remote!.remoteUri}: ${myClone.cloneTimeMs / 1000}s (bundle URI) vs. ${otherClone.cloneTimeMs / 1000}s (no bundle URI)`)
++ console.log(`\nClone execution time for ${this.remote!.remoteUri}: ${(myClone.cloneTimeMs / 1000).toFixed(2)}s (bundle URI) vs. ${(otherClone.cloneTimeMs / 1000).toFixed(2)}s (no bundle URI)`)
+})
-
- ## test/e2e/package.json ##
-@@
- "description": "",
- "main": "index.js",
- "scripts": {
-- "test": "cucumber-js"
-+ "test": "cucumber-js",
-+ "test-all": "cucumber-js -p all"
- },
- "license": "MIT",
- "devDependencies": {
4: 3d477c9 < -: ------- workflows: update 'go-version' to 'stable'
-: ------- > 4: 9c7860f e2e: add profile for running offline tests
-: ------- > 5: 704f46f workflows: use go version specified in 'go.mod'
5: cfcf0b0 ! 6: a6ff664 Makefile: add 'e2e-test' target
@@ Commit message
Add a target to the Makefile that installs the end-to-end test NPM
dependencies then runs the tests. By default, this will only run tests
matching the "default" profile of the tests (excluding performance tests).
- The 'TEST_E2E_PERF' flag, if it has a value, will instead run the tests with
- the 'all' profile (including the performance tests).
+ The 'E2E_FLAGS' variable can be used to specify the profile(s) used by the
+ tests (e.g., '--all' to include the tests excluded by default).
+
+ Add documentation on how to run the end-to-end tests to 'README.md'.
Signed-off-by: Victoria Dye <vdye@github.com>
@@ Makefile: PACKAGE_ARCH := $(GOARCH)
APPLE_APP_IDENTITY =
APPLE_INST_IDENTITY =
APPLE_KEYCHAIN_PROFILE =
-+TEST_E2E_PERF=
++E2E_FLAGS=
# Build targets
.PHONY: build
@@ Makefile: doc:
+ @echo
+ @echo "======== Running end-to-end tests ========"
+ $(RM) -r $(TESTDIR)
-+ @if [ -n "$(TEST_E2E_PERF)" ]; then \
-+ ./scripts/run-e2e-tests.sh --all; \
-+ else \
-+ ./scripts/run-e2e-tests.sh; \
-+ fi
++ @scripts/run-e2e-tests.sh $(E2E_FLAGS)
+
# Installation targets
.PHONY: install
@@ Makefile: clean:
$(RM) -r $(DOCDIR)
+ $(RM) -r $(TESTDIR)
+ ## README.md ##
+@@ README.md: $ go build -o bin/ ./...
+
+ ### Testing and Linting
+
+-To run the project's unit tests, navigate to the repository root directory and
+-run `go test -v ./...`.
++Unless otherwise specified, run commands from the repository root.
+
+-To run the project's linter, navigate to the repository root directory and run
+-`go vet ./...`.
++#### Unit tests
++
++```
++go test -v ./...
++```
++
++#### Linter
++
++```
++go vet ./...
++```
++
++#### End-to-end tests
++
++In order to run these tests, you need to have a recent version of
++[Node.js](https://nodejs.org) (current LTS version is a pretty safe bet) and NPM
++installed.
++
++For the standard set of tests (i.e., excluding exceptionally slow tests), run:
++
++```
++make e2e-test
++```
++
++To configure the test execution and filtering, set the `E2E_FLAGS` build
++variable. The available options are:
++
++* `--offline`: run all tests except those that require internet access.
++* `--all`: run all tests, including slow performance tests.
++
++The above modes are mutually exclusive; if multiple are specified, only the last
++will be used. For example, `E2E_FLAGS="--offline --all"` is equivalent to
++`E2E_FLAGS="--all"`.
++
++:warning: The performance tests that are excluded by default clone very large
++repos from the internet and can take anywhere from ~30 minutes to multiple hours
++to run, depending on internet connectivity and other system resources.
+
+ ## License
+
+
## scripts/run-e2e-tests.sh (new) ##
@@
+#!/bin/bash
@@ scripts/run-e2e-tests.sh (new)
+TESTDIR="$THISDIR/../test/e2e"
+
+# Defaults
-+NPM_TARGET="test"
++ARGS=()
+
+# Parse script arguments
+for i in "$@"
+do
+case "$i" in
++ --offline)
++ ARGS+=("-p" "offline")
++ shift # past argument
++ ;;
+ --all)
-+ NPM_TARGET="test-all"
++ ARGS+=("-p" "all")
+ shift # past argument
+ ;;
+ *)
@@ scripts/run-e2e-tests.sh (new)
+cd "$TESTDIR"
+
+npm install
-+npm run $NPM_TARGET
++npm run test -- ${ARGS:+${ARGS[*]}}
6: 617da16 ! 7: f0b17de workflows: add a workflow to run end-to-end tests
@@ Commit message
Add a dedicated 'test.yml' workflow that executes the end-to-end tests.
Unlike CI, these tests are only run automatically for pull requests
(although they can be manually triggered via workflow dispatch). By default,
- the tests will run with the "default" (no perf test) profile; this can be
- changed via an input variable in the workflow dispatch.
+ the tests will run with the "default" profile; this can be changed via an
+ input variable in the workflow dispatch.
Because the end-to-end tests deal with external dependencies (namely, Git),
add an 'install-dependencies.sh' script to install and configure the
@@ .github/workflows/test.yml (new)
+ pull_request:
+ workflow_dispatch:
+ inputs:
-+ test-perf:
++ run-all:
+ type: boolean
-+ description: 'Whether to run the (slow) end-to-end perf tests'
++ description: 'Include tests that are excluded by default due to slowness'
+ default: false
+
+jobs:
@@ .github/workflows/test.yml (new)
+ runs-on: ubuntu-latest
+
+ steps:
++ - name: Clone repository
++ uses: actions/checkout@v3
++
+ - name: Setup Go
+ uses: actions/setup-go@v3
+ with:
-+ go-version: 'stable'
++ go-version-file: 'go.mod'
+
+ - name: Setup Node.js
+ uses: actions/setup-node@v3
+ with:
+ node-version: 18
+
-+ - name: Clone repository
-+ uses: actions/checkout@v3
-+
+ - name: Install system dependencies for end-to-end tests
+ run: build/ci/install-dependencies.sh
+ shell: bash
+
+ - name: Enable perf tests
-+ if: ${{ github.event.inputs.test-perf }}
-+ run: echo "TEST_E2E_PERF=1" >> $GITHUB_ENV
++ if: ${{ github.event.inputs.run-all }}
++ run: echo "E2E_FLAGS='--all'" >> $GITHUB_ENV
+
+ - name: Run end-to-end tests
+ env:
-+ TEST_E2E_PERF: ${{matrix.jobs.goarch}}
-+ run: make e2e-test TEST_E2E_PERF="$TEST_E2E_PERF"
++ E2E_FLAGS: ${{env.E2E_FLAGS}}
++ run: make e2e-test E2E_FLAGS="$E2E_FLAGS"
## build/ci/install-dependencies.sh (new) ##
@@ |
derrickstolee
approved these changes
Mar 20, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #23
Summary
This pull request creates a Cucumber/TypeScript end-to-end testing framework for the Git Bundle Server (see #23 for background on the framework & reasoning for why TypeScript was selected). Two classes of tests are created to start with: "basic" tests (demonstrating expected typical workflows) and "performance" tests (comparing performance with & without use of the bundle server on large repositories).
The commits are broken down as follows:
e2e-testtarget to theMakefiletest.ymlworkflowPerformance results
The performance tests are quite slow, so they aren't run by default in the Actions workflow. For reference, some example results on my machine were:
Note that this only compares the clone time; it also took
torvalds/linux~10 minutes to clone & generate its 4.1Gb(!) base bundle. Since that scale is more reflective of the monorepos this bundle server would be used with than the small repos used in the "basic" tests, it's probably worth doing some more detailed profiling on the bundle server. Hopefully, we'd find some places where we can improve performance.