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

oasis-test-runner: Use explicit flagsets for scenario parameters #2937

Merged
merged 1 commit into from
May 29, 2020

Conversation

matevz
Copy link
Member

@matevz matevz commented May 22, 2020

Fixes #2897

@matevz matevz force-pushed the matevz/bug/test-runner-default-values branch from cded8f0 to 0e1f9f8 Compare May 22, 2020 15:16
@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #2937 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2937      +/-   ##
==========================================
+ Coverage   68.48%   68.56%   +0.07%     
==========================================
  Files         361      361              
  Lines       35131    35131              
==========================================
+ Hits        24060    24086      +26     
+ Misses       7978     7970       -8     
+ Partials     3093     3075      -18     
Impacted Files Coverage Δ
go/worker/compute/executor/committee/state.go 74.07% <0.00%> (-11.12%) ⬇️
go/common/grpc/policy/policy.go 64.38% <0.00%> (-6.85%) ⬇️
go/common/grpc/proxy/proxy.go 62.60% <0.00%> (-5.22%) ⬇️
go/storage/mkvs/tree.go 89.28% <0.00%> (-3.58%) ⬇️
go/worker/compute/executor/committee/node.go 61.55% <0.00%> (-3.31%) ⬇️
go/consensus/tendermint/roothash/roothash.go 66.21% <0.00%> (-2.37%) ⬇️
go/storage/api/root_cache.go 72.41% <0.00%> (-2.30%) ⬇️
go/storage/database/database.go 72.26% <0.00%> (-1.69%) ⬇️
go/storage/client/client.go 76.21% <0.00%> (-0.89%) ⬇️
go/roothash/tests/tester.go 95.58% <0.00%> (-0.64%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9afb05f...f51636e. Read the comment docs.

@kostko kostko requested a review from ptrus May 25, 2020 12:56
@matevz
Copy link
Member Author

matevz commented May 26, 2020

Cloning scenario and its flagset is not as simple as I thought. Doing

newSc.flags =  flag.NewFlagSet(sc.name, flag.ContinueOnError)
newSc.flags.AddFlagSet(sc.flags)

keeps old references to values so the flagset is not correctly cloned. I will need to make separate helpers per scenario for copying the flags...

@matevz
Copy link
Member Author

matevz commented May 26, 2020

Cloning scenario and its flagset is not as simple as I thought. Doing

newSc.flags =  flag.NewFlagSet(sc.name, flag.ContinueOnError)
newSc.flags.AddFlagSet(sc.flags)

keeps old references to values so the flagset is not correctly cloned. I will need to make separate helpers per scenario for copying the flags...

This was now solved in d676397

go/oasis-test-runner/scenario/e2e/e2e.go Outdated Show resolved Hide resolved
go/oasis-test-runner/scenario/e2e/e2e.go Outdated Show resolved Hide resolved
@matevz matevz force-pushed the matevz/bug/test-runner-default-values branch 5 times, most recently from 2b8d28f to fa76c30 Compare May 28, 2020 09:22
@matevz matevz force-pushed the matevz/bug/test-runner-default-values branch 3 times, most recently from 2fb18b0 to 484e701 Compare May 28, 2020 11:56
go/oasis-test-runner/env/env.go Show resolved Hide resolved
go/oasis-test-runner/scenario/e2e/runtime.go Outdated Show resolved Hide resolved
@matevz matevz force-pushed the matevz/bug/test-runner-default-values branch 2 times, most recently from f21f57d to a01eddb Compare May 28, 2020 14:27
@matevz matevz force-pushed the matevz/bug/test-runner-default-values branch from 4df8eb2 to f51636e Compare May 28, 2020 16:02
@matevz matevz merged commit f8aaea0 into master May 29, 2020
@matevz matevz deleted the matevz/bug/test-runner-default-values branch May 29, 2020 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set default values for E2E Scenarios Parameters
3 participants