Skip to content

ci: Fix FilterLine test matchers and related specs#11794

Merged
nebril merged 1 commit intocilium:masterfrom
ap4y:audit-mode-verdict
Jun 2, 2020
Merged

ci: Fix FilterLine test matchers and related specs#11794
nebril merged 1 commit intocilium:masterfrom
ap4y:audit-mode-verdict

Conversation

@ap4y
Copy link
Copy Markdown
Contributor

@ap4y ap4y commented May 31, 2020

Current matchers implementation makes policy related specs always pass
even when value is present in the output. This patch updates
ExpectContainsFilterLine and ExpectDoesNotContainFilterLine matchers
to match expected value against string slice rather than slice of
FilterBuffer. This patch also updates policy specs to use
ExpectContainsFilterLine matcher where applicable. Minor consistency
changes were also added to policy specs.

Current matchers implementation makes policy related specs always pass
even when value is present in the output. This patch updates
ExpectContainsFilterLine and ExpectDoesNotContainFilterLine matchers
to match expected value against string slice rather than slice of
FilterBuffer. This patch also updates policy specs to use
ExpectContainsFilterLine matcher where applicable. Minor consistency
changes were also added to policy specs.

Signed-off-by: Arthur Evstifeev <aevstifeev@gitlab.com>
@ap4y ap4y requested a review from a team as a code owner May 31, 2020 09:08
@maintainer-s-little-helper

This comment has been minimized.

Copy link
Copy Markdown
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Good catch, thank you!

I used WaitUntilMatchFilterLine instead of ExpectContainsFilterLine to check for the subsequent events because (in theory) they might not have been captured yet at the time of check. However, considering that the time window between the first and second event is in the order of microseconds and the fact that the old monitor based tests didn't seem to have the issue either, I'm happy to simplify the tests again.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.006%) to 36.909% when pulling 191ee8b on ap4y:audit-mode-verdict into b7be1c0 on cilium:master.

@gandro
Copy link
Copy Markdown
Member

gandro commented May 31, 2020

test-me-please

Edit: Known flakes unrelated to the PR (both in K8s, this PR touches Runtime).
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Kernel/1828/
https://jenkins.cilium.io/job/Cilium-PR-K8s-oldest-net-next/503/

@ap4y ap4y changed the title Fix FilterLine test matchers and related specs ci: Fix FilterLine test matchers and related specs May 31, 2020
@ap4y
Copy link
Copy Markdown
Contributor Author

ap4y commented May 31, 2020

@gandro I have reversed matchers a bit in egress test (wait on trace reply and check policy-log) to prevent this potential race condition. I have tested this few times and it seems to be working well.

@gandro
Copy link
Copy Markdown
Member

gandro commented May 31, 2020

@gandro I have reversed matchers a bit in egress test (wait on trace reply and check policy-log) to prevent this potential race condition. I have tested this few times and it seems to be working well.

Oh, I did not catch that! Even better then, thank you!

@gandro
Copy link
Copy Markdown
Member

gandro commented Jun 2, 2020

retest-4.9 (edit: note, this should have been 4.19)

@gandro
Copy link
Copy Markdown
Member

gandro commented Jun 2, 2020

retest-net-next

@gandro
Copy link
Copy Markdown
Member

gandro commented Jun 2, 2020

retest-4.19

@nebril
Copy link
Copy Markdown
Member

nebril commented Jun 2, 2020

net-next failed on unrelated flake, merging

1 similar comment
@nebril
Copy link
Copy Markdown
Member

nebril commented Jun 2, 2020

net-next failed on unrelated flake, merging

@nebril nebril merged commit a20dcd1 into cilium:master Jun 2, 2020
@ap4y ap4y deleted the audit-mode-verdict branch June 2, 2020 21:32
@gandro
Copy link
Copy Markdown
Member

gandro commented Jul 13, 2020

Looks like this never made it into the v1.8 branch. But we are using it in some Hubble tests (e.g. regression test) that we sometimes backport along with bug fixes. Marking for backport.

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

Labels

area/CI Continuous Integration testing issue or flake release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants