Skip to content

BaseIntegrationTest -> HttpIntegrationTest#1618

Merged
alyssawilk merged 7 commits intoenvoyproxy:masterfrom
alyssawilk:version
Sep 12, 2017
Merged

BaseIntegrationTest -> HttpIntegrationTest#1618
alyssawilk merged 7 commits intoenvoyproxy:masterfrom
alyssawilk:version

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

Moving the bulk of HTTP/H2-specific logic into a subclass of BaseIntegrationTest

Also moving client_protocol_ into the new HttpIntegrationTest

Copy link
Copy Markdown
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.

Nice clean up!

Comment thread test/integration/integration.cc Outdated
}

void BaseIntegrationTest::sendRequestAndWaitForResponse(Http::TestHeaderMapImpl& request_headers,
HttpIntegrationTest::HttpIntegrationTest(Http::CodecClient::Type client_protocol,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you want to split/rename to http_integation_test.cc etc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. I figured diffs would be easier this way, but either way it'll be separate commits. Will do in the next push :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Split out but I'll admit I was lazy cleaning up includes and libraries, given it's test code.
Feel free to tell me to be non-lazy if you care about such things, otherwise I'll just make sure the OSX build doesn't choke.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My preference is to try to keep the test code cleanly as we do with the production code, if you don't mind doing those cleanups. If you are too busy I would potentially just add a TODO for the cleanup.


namespace Envoy {
class Http2IntegrationTest : public BaseIntegrationTest,
class Http2IntegrationTest : public HttpIntegrationTest,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While we're thinking about refactoring, protocols and programmatic generation of config, what are your thoughts about being able to independently set the downstream/upstream protocol? Today, upstream is essentially a property of how we setup the config file, rather than test driven. It seems that (just like in GFE), we want to be able to treat these both as first class properties in tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that one may have to want until backend config is proto-driven, but I planned to experiment with it next.

Copy link
Copy Markdown
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.

very nice!

Comment thread test/integration/BUILD Outdated
hdrs = [
"http_integration.h",
],
data = ["//test/common/runtime:filesystem_test_data"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this needed? I don't remember history here.

mattklein123
mattklein123 previously approved these changes Sep 11, 2017
Copy link
Copy Markdown
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 after merge.

@alyssawilk alyssawilk merged commit 974718a into envoyproxy:master Sep 12, 2017
@alyssawilk alyssawilk deleted the version branch September 12, 2017 15:37
mathetake pushed a commit that referenced this pull request Mar 3, 2026
**Description**

This commit adds indexing for referencegrants. The indexing key uses To
Kind + namespace in referencegrant for indexing which would help fetch
only related referencegrants given aiservicebackend and namespace.

**Related Issues/PRs (if applicable)**
Closes #1375

---------

Signed-off-by: siddharth1036 <siddharthshah1036@gmail.com>
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.

3 participants