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: multiple skips #148

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
17 changes: 16 additions & 1 deletion .github/actions/test/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ inputs:
description: "A comma-separated list of specs to be tested. Accepts a spec (test only this spec), a +spec (test also this immature spec), or a -spec (do not test this mature spec)."
required: false
default: ""
skips:
description: "A json array of regexp to skip tests."
required: false
default: ""
args:
description: "[DANGER] The `args` input allows you to pass custom, free-text arguments directly to the Go test command that the tool employs to execute tests."
required: false
Expand All @@ -33,19 +37,30 @@ runs:
steps:
- id: github
uses: pl-strflt/docker-container-action/.github/actions/github@v1
- id: prepare-args
shell: bash
env:
SKIPS: ${{ inputs.skips }}
run: |
SKIPS_ARGS=""
if [ -n "$SKIPS" ]; then
SKIPS_ARGS=$(echo "$SKIPS" | jq -r '.[]' | sed -e 's/^/--skip="/' -e 's/$/"/' | paste -s -d ' ' -)
fi
echo "SKIPS_ARGS=$SKIPS_ARGS" >> $GITHUB_OUTPUT
- name: Run the test
uses: pl-strflt/docker-container-action@v1
env:
URL: ${{ inputs.gateway-url }}
SUBDOMAIN: ${{ inputs.subdomain-url }}
JSON: ${{ inputs.json }}
SPECS: ${{ inputs.specs }}
SKIPS: ${{ steps.prepare-args.outputs.SKIPS_ARGS }}
with:
repository: ${{ steps.github.outputs.action_repository }}
ref: ${{ steps.github.outputs.action_sha || steps.github.outputs.action_ref }}
dockerfile: Dockerfile
opts: --network=host
args: test --url="$URL" --json="$JSON" --specs="$SPECS" --subdomain-url="$SUBDOMAIN" -- ${{ inputs.args }}
args: test --url="$URL" --json="$JSON" --specs="$SPECS" ${SKIPS} --subdomain-url="$SUBDOMAIN" -- ${{ inputs.args }}
build-args: |
VERSION:${{ steps.github.outputs.action_ref }}
- name: Create the XML
Expand Down
81 changes: 81 additions & 0 deletions .github/workflows/test-kubo-skipped-e2e.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
name: Test Kubo Skipped (e2e)

on:
workflow_dispatch:
push:
branches:
- main
pull_request:

jobs:
test:
runs-on: 'ubuntu-latest'
strategy:
fail-fast: false
matrix:
target: ['latest', 'master']
defaults:
run:
shell: bash
steps:
- name: Setup Go
uses: actions/setup-go@v3
with:
go-version: 1.20.4
- uses: actions/checkout@v3
with:
path: 'gateway-conformance'
- name: Extract fixtures
uses: ./gateway-conformance/.github/actions/extract-fixtures
with:
output: fixtures
- uses: protocol/cache-go-action@v1
- run: go install github.com/ipfs/kubo/cmd/ipfs@${{ matrix.target }}
shell: bash
env:
GOPROXY: direct
- name: Configure Kubo Gateway
run: |
ipfs init;
source ./gateway-conformance/kubo-config.example.sh "$(pwd)/fixtures"
echo "IPFS_NS_MAP=${IPFS_NS_MAP}" >> $GITHUB_ENV
# note: the IPFS_NS_MAP set above will be passed the daemon
- uses: ipfs/start-ipfs-daemon-action@v1
with:
args: '--offline'
wait-for-addrs: false
- name: Provision Kubo Gateway
run: |
# Import car files
cars=$(find ./fixtures -name '*.car')
for car in $cars
do
ipfs dag import --pin-roots=false --stats "$car"
done

# Import ipns records
records=$(find ./fixtures -name '*.ipns-record')
for record in $records
do
key=$(basename -s .ipns-record "$record" | cut -d'_' -f1)
ipfs routing put --allow-offline "/ipns/$key" "$record"
done
- name: Run the tests
uses: ./gateway-conformance/.github/actions/test
with:
gateway-url: http://127.0.0.1:8080
subdomain-url: http://example.com
json: output.json
xml: output.xml
html: output.html
markdown: output.md
skips: '["TestTrustlessCarPathing", "TestNativeDag/GET_plain_JSON_codec_from_.*"]'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Highlight: this is how we skip tests. Note the regexp similar to golang's matching

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same as the following, right?

Suggested change
skips: '["TestTrustlessCarPathing", "TestNativeDag/GET_plain_JSON_codec_from_.*"]'
args: '-skip "(TestTrustlessCarPathing|TestNativeDag/GET_plain_JSON_codec_from_.*)"'

Copy link
Member

@lidel lidel Aug 22, 2023

Choose a reason for hiding this comment

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

Regex support is pretty recent, i think? https://tip.golang.org/doc/go1.20 states:

The go test command now accepts -skip to skip tests, subtests, or examples matching .

go help testflag conforms we now have parity for -run and -skip which is really nice!:

-run regexp
	    Run only those tests, examples, and fuzz tests matching the regular
	    expression. For tests, the regular expression is split by unbracketed
	    slash (/) characters into a sequence of regular expressions, and each
	    part of a test's identifier must match the corresponding element in
	    the sequence, if any. Note that possible parents of matches are
	    run too, so that -run=X/Y matches and runs and reports the result
	    of all tests matching X, even those without sub-tests matching Y,
	    because it must run them to look for those sub-tests.
	    See also -skip.
[..]
	-skip regexp
	    Run only those tests, examples, fuzz tests, and benchmarks that
	    do not match the regular expression. Like for -run and -bench,
	    for tests and benchmarks, the regular expression is split by unbracketed
	    slash (/) characters into a sequence of regular expressions, and each
	    part of a test's identifier must match the corresponding element in
	    the sequence, if any.

Both skip and run are useful. run allows more fine-grained control than our predefined presets, which saves time when someone develops a fix and has to re-run specific subset of tests without running the entire suite (not a problem today, but the suite will grow).

@laurentsenta do we need to have dedicated input in github action?
If we could keep this under args we would not have to do anything special to support -run here, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also wondering if we even need dedicated run/skip gateway conformance cli flags if we can accomplish the same using the go flags.

If we do, e.g. as nicer to use sugar, I think it might be worth considering converting the custom flag values into regex and passing it to go to avoid having to reimplement run/skip logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Thanks for the reviews and comments,
That work might be a dead end, Sorry for the noise 🤦

I ran more experiments, and not everything makes sense to me, but regular skips should be enough.

@galargh your suggestion would need a different parenthesizing:

(TestTrustlessCarPathing|TestNativeDag/GET_plain_JSON_codec_from_.*) # does not work
(TestTrustlessCarPathing)|(TestNativeDag/GET_plain_JSON_codec_from_.*) # would work

@lidel your issue should be solved with an | and parenthesis:

go test ... -skip '(TestGatewayCache/GET_for_/ipfs/_with_only-if-cached_succeeds_when_in_local_datastore)|(TestGatewayCache/HEAD_for_/ipfs/_with_only-if-cached_succeeds_when_in_local_datastore)'

Probably also:

go test ... -skip 'TestGatewayCache/(GET|HEAD)_for_/ipfs/_with_only-if-cached_succeeds_when_in_local_datastore'

We can drop that PR.

I think the easiest way to think about golang -skip is:

  • create groups with (g1)|(g2) at the top level,
  • then each group is split by / and each part is matched with the go test name.
    • willSkip("regexp1/regexp2", "testX/testY") = match(regexp1, testX) && match(regexp2, testY)

I'm still confused by some aspects of the matching... if you want to dig deeper: https://gist.github.com/laurentsenta/5ed9fe9954605eefc1ed9ee2db7f66a9

But I'll close this PR for now and continue the discussion in #141.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for looking into it 🙇 That's a nice write-up for a tricky feature 👏

- name: Set summary
if: (failure() || success())
run: cat ./output.md >> $GITHUB_STEP_SUMMARY
- name: Upload one-page HTML report
if: (failure() || success())
uses: actions/upload-artifact@v3
with:
name: conformance-${{ matrix.target }}.html
path: ./output.html
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## Unreleased
### Added
- `--version` flag shows the current version
- `--skip` parameter to skip one or more tests. [PR](https://github.com/ipfs/gateway-conformance/pull/148)
- Metadata logging used to associate tests with custom data like versions, specs identifiers, etc.

## [0.3.0] - 2023-07-31
Expand Down
7 changes: 5 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ The `test` command is the main command of the tool. It is used to test a given I
| html | GitHub Action | The path where the one-page HTML test report should be generated. | `./report.html` |
| markdown | GitHub Action | The path where the summary Markdown test report should be generated. | `./report.md` |
| specs | Both | A comma-separated list of specs to be tested. Accepts a spec (test only this spec), a +spec (test also this immature spec), or a -spec (do not test this mature spec). | Mature specs only |
| skip | Both | Run only the those tests that do not match the regular expression. Similar to golang's skip, the expression is split by slash (/) into a sequence and each part must match the corresponding part in the test name, if any | empty |
| args | Both | [DANGER] The `args` input allows you to pass custom, free-text arguments directly to the Go test command that the tool employs to execute tests. | N/A |

##### Specs
Expand Down Expand Up @@ -79,6 +80,7 @@ A few examples:
markdown: report.md
html: report.html
args: -timeout 30m
skips: '["TestGatewaySubdomains", "TestGatewayCar/GET_response_for_application/.*/Header_Content-Length"]'
```

##### Docker
Expand Down Expand Up @@ -235,10 +237,11 @@ gateway-conformance test --specs trustless-gateway,-trustless-gateway-ipns

### Skip a specific test
Copy link
Member

@lidel lidel Aug 22, 2023

Choose a reason for hiding this comment

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

Could we add ### Run only a specific test with example that uses go's go test -run <pattern> and use the same pattern as in the skip example?

Include this note:

The -run argument uses the same syntax as go test -run and gives more fine-grained control than predefined presets passed via --specs. It aims to save time when developing a fix and having to re-run specific subset of tests without running the entire suite.


Tests are skipped using Go's standard syntax:
Tests are skipped using the `--skip` parameter and Go's standard syntax:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Tests are skipped using the `--skip` parameter and Go's standard syntax:
Tests can be skipped using the `--skip` parameter which follows `go test -skip regex` syntax, but can be passed more than once for convenience:


```bash
gateway-conformance test -skip 'TestGatewayCar/GET_response_for_application/vnd.ipld.car/Header_Content-Length'
gateway-conformance test --skip 'TestGatewayCar/GET_response_for_application/vnd.ipld.car/Header_Content-Length'
gateway-conformance test --skip 'TestGatewayCar/.*/vnd.ipld.car' --skip 'TestGatewayCar/GET_response_for_application/vnd.*'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

highlight: use in CLI

```

### Extracting the test fixtures
Expand Down
18 changes: 17 additions & 1 deletion cmd/gateway-conformance/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"bytes"
"encoding/json"
"fmt"
"io"
"log"
Expand Down Expand Up @@ -76,6 +77,7 @@ func main() {
var directory string
var merged bool
var verbose bool
var skips cli.StringSlice

app := &cli.App{
Name: "gateway-conformance",
Expand Down Expand Up @@ -113,6 +115,13 @@ func main() {
Value: "",
Destination: &specs,
},
&cli.StringSliceFlag{
Name: "skip",
Usage: "Accepts a test path to skip.\nCan be used multiple times.\n" +
"Example: --skip \"TestTar/GET_TAR_with_format=.*/Body\" --skip \"TestGatewayBlock\" --skip \"TestTrustlessCarPathing\"\n" +
"It uses the same syntax as the -skip flag of go test.",
Destination: &skips,
},
Comment on lines +118 to +124
Copy link
Member

Choose a reason for hiding this comment

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

Parity with go-test -skip is 👍
Could we include the same for go-test -run in this PR?
It will make things feature-complete, and natural for anyone familiar with go, and at least remove surprises when someone tries -skip and -run – these should work the same, and if we support passing -skip multiple times, we should also support that for -run 🙏

&cli.BoolFlag{
Name: "verbose",
Usage: "Prints all the output to the console.",
Expand All @@ -134,10 +143,17 @@ func main() {

fmt.Println("go " + strings.Join(args, " "))

// json encode the string array for parameter
skipsList := skips.Value()
skipsJSON, err := json.Marshal(skipsList)
if err != nil {
return fmt.Errorf("failed to marshal skips: %w", err)
}

output := &bytes.Buffer{}
cmd := exec.Command("go", args...)
cmd.Dir = tooling.Home()
cmd.Env = append(os.Environ(), fmt.Sprintf("GATEWAY_URL=%s", gatewayURL))
cmd.Env = append(os.Environ(), fmt.Sprintf("GATEWAY_URL=%s", gatewayURL), fmt.Sprintf("TEST_SKIPS=%s", skipsJSON))

if subdomainGatewayURL != "" {
cmd.Env = append(cmd.Env, fmt.Sprintf("SUBDOMAIN_GATEWAY_URL=%s", subdomainGatewayURL))
Expand Down
115 changes: 115 additions & 0 deletions tooling/test/skips.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package test

import (
"encoding/json"
"os"
"regexp"
"testing"
)

const SKIPS_ENV_VAR = "TEST_SKIPS"

var skips = []string{}

func GetSkips() []string {
skips := os.Getenv(SKIPS_ENV_VAR)
if skips == "" {
return []string{}
}

var skipsList []string
err := json.Unmarshal([]byte(skips), &skipsList)
if err != nil {
panic(err)
}

return skipsList
}

func split(name string) []string {
// split name by "/" not prefixed with "\"
parts := []string{}
current := ""
hasSlash := false

for _, c := range name {
if hasSlash {
if c == '/' || c == '\\' {
current += string(c)
hasSlash = false
} else {
current += "\\"
current += string(c)
hasSlash = false
}
continue
}

if c == '/' {
parts = append(parts, current)
current = ""
continue
}
if c == '\\' {
hasSlash = true
continue
}

current += string(c)
}

if current != "" {
parts = append(parts, current)
}

return parts
}

func isSkipped(name string, skips []string) bool {
for _, skip := range skips {
if name == skip {
return true
}

skipParts := split(skip)
nameParts := split(name)

if len(skipParts) > len(nameParts) {
continue
}

matches := true
for i := range skipParts {
skipPart := skipParts[i]
skipPart = "^" + skipPart + "$"

matched, err := regexp.MatchString(skipPart, nameParts[i])
if err != nil {
panic(err)
}

if !matched {
matches = false
break
}
}

return matches
}

return false
}

func MustNotBeSkipped(t *testing.T) {
t.Helper()
skipped := isSkipped(t.Name(), skips)

if skipped {
t.Skipf("skipped")
}
}

// init will load the skips
func init() {
skips = GetSkips()
}
Loading