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

test: making assert required #15972

Merged
merged 4 commits into from
Apr 15, 2021
Merged

test: making assert required #15972

merged 4 commits into from
Apr 15, 2021

Conversation

alyssawilk
Copy link
Contributor

follow-up on #15812 and envoyproxy/envoy-filter-example#143

Risk Level: n/a (test only)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

htuch
htuch previously approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch self-assigned this Apr 14, 2021
@alyssawilk
test: making assert required
a600850 
Signed-off-by: Alyssa Wilk <[email protected]>
@alyssawilk
Copy link
Contributor Author

apologies for force push - screwed up local state somehow

@alyssawilk
bump envoy filter
1bcbf60 
Signed-off-by: Alyssa Wilk <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15972 was synchronize by alyssawilk.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 14, 2021
@qinggniq
Copy link
Contributor

@alyssawilk Hello, have you solved the ci flakes of http_filter_integration_test at ci asan or tsan? My pr #15803 is rely on it. I change ci/filter_example_setup.sh, set SHA to my commit at my fork repo, but it failed at http_filter_integration_test. After test, I found this commit lead to the failure of test.

@qinggniq qinggniq mentioned this pull request Apr 15, 2021
Merged
@alyssawilk
Merge branch 'main' into required
81d7d74 
Signed-off-by: Alyssa Wilk <[email protected]>
@alyssawilk
bump again
afb8e6a 
Signed-off-by: Alyssa Wilk <[email protected]>
@alyssawilk
Copy link
Contributor Author

@qinggniq sorry about that - ironically the new ASSERT was designed to catch timeouts like this, I just didn't realize that test was already broken!
envoyproxy/envoy-filter-example#146 should hopefully fix but if not I'll get it sorted out somehow today, either by reverting the two changes or actually getting that test working as it was intended to!

@alyssawilk
Copy link
Contributor Author

To confirm, CI is happy with this hash, sorry about the temporary breakage!

mattklein123
mattklein123 approved these changes Apr 15, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice!

@mattklein123 mattklein123 merged commit e2eebec into envoyproxy:main Apr 15, 2021
@qqustc qqustc mentioned this pull request Apr 19, 2021
Merged
douglas-reid pushed a commit to douglas-reid/envoy that referenced this pull request Apr 19, 2021
@alyssawilk @douglas-reid
test: making assert required (envoyproxy#15972)
f47e440 
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Douglas Reid <[email protected]>
Monkeyanator pushed a commit to Monkeyanator/envoy that referenced this pull request Apr 20, 2021
@alyssawilk
test: making assert required (envoyproxy#15972)
90f5114 
Signed-off-by: Alyssa Wilk <[email protected]>
mum4k pushed a commit to envoyproxy/nighthawk that referenced this pull request Apr 20, 2021
@qqustc
Update Envoy to 751be72 (April 19th 2021) (#674)
b2e7dca 
- Sync .bazelrc
- Update Docker image version in .circleci/config.yml
- ci/run_envoy_docker.sh was unchanged
- Code changes
  - Remove `response->waitForEndStream();` from `HttpFilterIntegrationTestBase::getResponse()` in test/server/http_filter_integration_test_base.cc due to envoyproxy/envoy#15972. We can't simply change it to `ASSERT_TRUE(response->waitForEndStream());` in `HttpFilterIntegrationTestBase::getResponse()` since this will cause error:
    ```
    test/server/http_filter_integration_test_base.cc:69:5: error: no viable conversion from returned value of type 'void' to function return type 'Envoy::IntegrationStreamDecoderPtr' (aka 'unique_ptr<Envoy::IntegrationStreamDecoder>')
        ASSERT_TRUE(response->waitForEndStream());
    ```
   - Added `ASSERT_TRUE(response->waitForEndStream());` to all places where `HttpFilterIntegrationTestBase::getResponse()` is called in the tests.

Signed-off-by: [email protected] <[email protected]>
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
@alyssawilk
test: making assert required (envoyproxy#15972)
3aaf964 
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Gokul Nair <[email protected]>
wjuan-AFK pushed a commit to wjuan-AFK/nighthawk that referenced this pull request May 11, 2021
@qqustc @wjuan-AFK
Update Envoy to 751be72 (April 19th 2021) (envoyproxy#674)
e2a7aff 
- Sync .bazelrc
- Update Docker image version in .circleci/config.yml
- ci/run_envoy_docker.sh was unchanged
- Code changes
  - Remove `response->waitForEndStream();` from `HttpFilterIntegrationTestBase::getResponse()` in test/server/http_filter_integration_test_base.cc due to envoyproxy/envoy#15972. We can't simply change it to `ASSERT_TRUE(response->waitForEndStream());` in `HttpFilterIntegrationTestBase::getResponse()` since this will cause error:
    ```
    test/server/http_filter_integration_test_base.cc:69:5: error: no viable conversion from returned value of type 'void' to function return type 'Envoy::IntegrationStreamDecoderPtr' (aka 'unique_ptr<Envoy::IntegrationStreamDecoder>')
        ASSERT_TRUE(response->waitForEndStream());
    ```
   - Added `ASSERT_TRUE(response->waitForEndStream());` to all places where `HttpFilterIntegrationTestBase::getResponse()` is called in the tests.

Signed-off-by: [email protected] <[email protected]>
Signed-off-by: William Juan <[email protected]>
@alyssawilk alyssawilk deleted the required branch February 28, 2022 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@htuch htuch htuch left review comments

@snowp snowp Awaiting requested review from snowp

Assignees

@htuch htuch

Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alyssawilk @qinggniq @mattklein123 @htuch