Skip to content

test: fix cli flags handling#12099

Merged
aanm merged 3 commits intomasterfrom
pr/fix-test-flags-handling
Jun 17, 2020
Merged

test: fix cli flags handling#12099
aanm merged 3 commits intomasterfrom
pr/fix-test-flags-handling

Conversation

@nebril
Copy link
Copy Markdown
Member

@nebril nebril commented Jun 16, 2020

Review per commit, this change fixes quarantine pipelines.

nebril added 3 commits June 16, 2020 14:38
This will allow us to read config variables in ginkgo-ext package before
ginkgo creates a test tree.

Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
Parsing cli flags in init func of our test package caused flag values to
be unavailable when ginkgo is creating test tree, which caused
`cilium.RunQuarantined` option to always be false.

This change moves flag parsing to a moment before ginkgo tree is
created.

Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
@nebril nebril requested a review from a team as a code owner June 16, 2020 12:45
@maintainer-s-little-helper
Copy link
Copy Markdown

Please set the appropriate release note label.

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 16, 2020

test-me-please

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 37.049% when pulling 381cb8c on pr/fix-test-flags-handling into 00ef71b on master.

Copy link
Copy Markdown
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me.
If I understand correctly, second commit (to parse just -cilium.* flags) is necessary because we'd parse the other arguments passed to ginkgo (like --focus) otherwise?

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 16, 2020

@qmonnet correct, other arguments are parsed by ginkgo and go test commands

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/ci This PR makes changes to the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants