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.

Sign up for GitHub

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

Jump to bottom

Fail the integration tests on warnings or errors. #581

Merged
merged 19 commits into from
Dec 8, 2020
Merged

Fail the integration tests on warnings or errors. #581

merged 19 commits into from
Dec 8, 2020

Conversation

mum4k
Copy link
Collaborator

@mum4k mum4k commented Dec 1, 2020 •
edited
Loading

Fails the integration tests if any unknown warnings or errors are found in the logs of the Nighthawk test server.
Includes an ignore list that (for now) accepts known warnings found in the logs, the ignore list can have different content per each test case.

Also:

  • moving the Attributes: docstring section on IntegrationTestBase into its class docstring. __init__ is just a method and should only have the Args: section.

Fixes #577

Signed-off-by: Jakub Sobon [email protected]

@mum4k mum4k requested review from oschaaf and wjuan-AFK December 1, 2020 05:19
@mum4k mum4k added the waiting-for-review A PR waiting for a review. label Dec 1, 2020
@mum4k
Copy link
Collaborator Author

mum4k commented Dec 1, 2020

@wjuan-AFK please review and assign back to me once done.

oschaaf
oschaaf reviewed Dec 1, 2020
View reviewed changes
Copy link
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

looks great, just two questions/thoughts.

@oschaaf oschaaf added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Dec 1, 2020
@mum4k mum4k added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Dec 2, 2020
@mum4k mum4k mentioned this pull request Dec 2, 2020
Open
@mum4k mum4k requested a review from dubious90 December 2, 2020 05:12
@mum4k
Copy link
Collaborator Author

mum4k commented Dec 2, 2020

Adding @dubious90 in case he can get to this sooner than @wjuan-AFK.

oschaaf
oschaaf previously approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

LGTM

@wjuan-AFK
Copy link
Contributor

LGTM

wjuan-AFK
wjuan-AFK previously approved these changes Dec 2, 2020
View reviewed changes
@mum4k mum4k dismissed stale reviews from wjuan-AFK and oschaaf via 78f54d1 December 3, 2020 02:49
@mum4k mum4k removed the waiting-for-review A PR waiting for a review. label Dec 3, 2020
mum4k added 11 commits December 2, 2020 22:20
@mum4k
Fail the integration tests on warnings or errors.
da34805 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Autoformatting Python files.
0e3d499 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Adding the caplog fixture to the python benchmarks.
725e405 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Updating TODO to list the tracking issue.
89f83a7 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Moving tests for our test code into a unit_test subdirectory.
f70cc55 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Simplifying the unit_tests/BUILD file.
cf37f03 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Also fail integration tests on ERRORs, not just WARNINGs.
dc4327e 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Fixing nameerror in the teardown method.
9133b9f 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Renaming a variable to a more readable form.
61821db 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Changing variable messages to a more readable name.
9173ba3 
Signed-off-by: Jakub Sobon <[email protected]>
@mum4k
Adjusting the list of ignored warnings.
420c2b6 
Removing all those related to the v2 bootstraps that are no longer
present.

Signed-off-by: Jakub Sobon <[email protected]>
@mum4k mum4k added the waiting-for-review A PR waiting for a review. label Dec 3, 2020
@mum4k
Copy link
Collaborator Author

mum4k commented Dec 3, 2020

@oschaaf please take another look after addressing feedback and merging from master. Sorry about the force push, I have fat-fingered a bad rebase into this branch and had to rewrite history to fix it. Same commits were re-applies as cherry-picks.

oschaaf
oschaaf previously approved these changes Dec 5, 2020
View reviewed changes
@mum4k
Copy link
Collaborator Author

mum4k commented Dec 8, 2020

@oschaaf please take another look after merging from master.

@mum4k
Copy link
Collaborator Author

mum4k commented Dec 8, 2020

@oschaaf fyi this PR needs bit more work. Now that we do have a test which verifies we pass with v2 config if the user requests it, we fail because that warning appears in the server logs. Going to update this so that we can filter out messages per specific set of test cases.

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Dec 8, 2020
@mum4k
Copy link
Collaborator Author

mum4k commented Dec 8, 2020

@oschaaf this is ready for another review. We can now specify distinct ignore lists based on the name of the executed test case. The test case that purposefully runs with the deprecated Envoy v2 API is now allowed to log the relevant warnings.

@mum4k mum4k added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Dec 8, 2020
@mum4k
Copy link
Collaborator Author

mum4k commented Dec 8, 2020 •
edited
Loading

@wjuan-AFK significant changes were made since your last review to allow this to pass tests after #584 was merged in. For your convenience, here is a view that only includes commits related to this change.

oschaaf
oschaaf approved these changes Dec 8, 2020
View reviewed changes
Copy link
Member

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

Nice, this will be very useful, LGTM

@wjuan-AFK
Copy link
Contributor

LGTM

@mum4k mum4k merged commit 30bd666 into envoyproxy:master Dec 8, 2020
@mum4k mum4k deleted the integ-fail-on-warn branch December 8, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oschaaf oschaaf oschaaf approved these changes

@wjuan-AFK wjuan-AFK wjuan-AFK left review comments

@dubious90 dubious90 Awaiting requested review from dubious90

Assignees
No one assigned
Labels
waiting-for-review A PR waiting for a review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integration tests should report warnings/errors in logs
3 participants
@mum4k @wjuan-AFK @oschaaf