Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: libp2p implementation interoperability dashboard on various dimensions. Uses testplans-dsl. #85

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/actions/setup-testground/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ inputs:
testground_ref:
required: false
default: 'edge'
start_testground:
required: false
default: 'true'

runs:
using: "composite"
Expand Down Expand Up @@ -50,6 +53,7 @@ runs:
shell: bash

- name: Run the daemon or configure the client
if: ${{ inputs.start_testground == 'true' }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, with this merged, nothing would require this to be true anymore, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing ping tests use the run-composition GH action which depends on this being true.

The multidim tests starts its own testground daemon so that we can control the home location and the .env.toml file. In this case we don't want the setup-testground action to start the testground daemon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing ping tests use the run-composition GH action which depends on this being true.

Do I understand correctly that long term that GH action would go away? Do I also understand correctly that we can not yet remove it here given that that would break CIs in rust-libp2p and go-libp2p?

For the sake of cleaning it up right away, would that breakage of our CI be that bad?

Copy link
Contributor Author

@MarcoPolo MarcoPolo Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that long term that GH action would go away?

I wonder if there's still value in these existing ping tests. We get most of the value from them in the new multidim interop tests. The other value is having a fast test that checks if current master works against the other implementations. This GH action would go away when we stop using the ping tests. Right now there's still some value in the ping tests that isn't captured by this PR.

Do I also understand correctly that we can not yet remove it here given that that would break CIs in rust-libp2p and go-libp2p?

Yeah, and we wouldn't have anything to replace them.

For the sake of cleaning it up right away, would that breakage of our CI be that bad?

We don't have a plan right now to replace the existing ping tests. I'd rather focus on replacing it and then update the other repo's CI rather than breaking CI and then forcing us to fix it in a reactive way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other value is having a fast test that checks if current master works against the other implementations.

That would as well happen via the multidimensional tests, no?

Right now there's still some value in the ping tests that isn't captured by this PR.

What value would they provide long term?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would as well happen via the multidimensional tests, no?

Long term yes. Right now, no.

What value would they provide long term?

Long term, we should get rid of them. Right now though, I think the priority is to have the multidim interop tests. Replacing the ping tests imo is lower priority.

What the ping test gives us that multidim doesn't right now:

  • CI testing in the impl repo.
  • Interop testing with Nim (I haven't spent any time getting Nim into the multidim)

Long term, multidim will do this and we can get rid of the ping tests. I would save that for a future PR rather than add to this one. And I would avoid removing the existing ping tests until we're ready to replace them.

shell: bash
env:
TESTGROUND_ENDPOINT: ${{ inputs.testground_endpoint }}
Expand All @@ -72,6 +76,7 @@ runs:
fi;

- name: Check testground daemon health
if: ${{ inputs.start_testground == 'true' }}
run:
echo "Waiting for Testground to launch on 8042...";
while ! nc -z localhost 8042; do
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/multidim-interop.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
on:
workflow_dispatch:
pull_request:

name: libp2p multidimensional interop test

jobs:
run-multidim-interop:
uses: "./.github/workflows/run-testplans-dsl.yml"
with:
dir: "multidim-interop"
79 changes: 79 additions & 0 deletions .github/workflows/run-testplans-dsl.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
name: Run composition file with a custom git reference

on:
workflow_call:
inputs:
dir:
description: the directory with the testplans DSL test
required: true
type: string
jobs:
run_test:
name: Run testplans test
runs-on: ubuntu-latest
env:
TEST_PLAN_DIR: ${{ inputs.dir }}
defaults:
run:
shell: bash
steps:
- name: Checkout sources
uses: actions/checkout@v2
with:
path: test-plans
repository: ${{ env.TEST_PLAN_REPO }}
ref: ${{ env.TEST_PLAN_BRANCH }}
- uses: actions/setup-node@v3
with:
node-version: 17
cache: 'npm'
cache-dependency-path: ./test-plans/${{ env.TEST_PLAN_DIR }}/package-lock.json
- name: Expose GitHub Runtime # Needed for docker buildx to cache properly (See https://docs.docker.com/build/cache/backends/gha/#authentication)
uses: crazy-max/ghaction-github-runtime@v2
- name: Set up Docker Buildx
id: buildx
uses: docker/setup-buildx-action@v2
- name: Install DSL deps
working-directory: ./test-plans/dsl/
run: npm ci
- name: Install deps
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: npm ci
- name: setup testground
uses: ./test-plans/.github/actions/setup-testground
with:
start_testground: "false"
- name: Build images
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: make
- name: List docker images
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: docker image ls
- name: Run the test
timeout-minutes: 120
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: PATH=~/testground:$PATH npm run test
- name: Print the results
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: cat results.csv
- name: Render results
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: npm run renderResults > ./dashboard.md
- name: Show Dashboard Output
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: cat ./dashboard.md >> $GITHUB_STEP_SUMMARY
- name: Exit with Error
working-directory: ./test-plans/${{ env.TEST_PLAN_DIR }}/
run: |
if grep -q ":red_circle:" ./dashboard.md; then
exit 1
else
exit 0
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That check might break silently if we change the dashboard format, we could use a different exit code in renderResults to mark the case "the render completed, but it contains an error".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer this to a future change.

- uses: actions/upload-artifact@v3
if: ${{ always() }}
with:
name: testground-output
path: |
./test-plans/${{ env.TEST_PLAN_DIR }}/results.csv
./test-plans/${{ env.TEST_PLAN_DIR }}/dashboard.md
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,7 @@ __pycache__/
.history

# End of https://www.gitignore.io/api/go,visualstudiocode

### NodeJS

node_modules
2 changes: 2 additions & 0 deletions dsl/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
node_modules
results.csv
Loading