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.

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

test: refactor header inclusion to speed up building (for test/common/...) #12046

Merged
merged 5 commits into from
Jul 18, 2020

Conversation

foreseeable
Copy link
Contributor

Signed-off-by: Muge Chen mugechen@google.com

Refactor mock class include directive after dividing test/server/mocks.h (#11649)

Since the refactoring affects ~100 files so I split it up to several pull requests (previous one #11952 #12045)

Building time comparison for affected tests: see spreadsheet at #10917 comment

Commit Message: refactor header inclusion to speed up building
Additional Description:
Risk Level: low
Testing: exsiting tests
Docs Changes: N/A
Release Notes: no
Related Issues: #10917

\cc @ahedberg @bbarenblat

Signed-off-by: Muge Chen <mugechen@google.com>
Copy link
Contributor

@ahedberg ahedberg left a comment

Choose a reason for hiding this comment

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

LGTM assuming CI looks good. Please merge upstream/master here too.

test/common/grpc/grpc_client_integration_test_harness.h Outdated Show resolved Hide resolved
Signed-off-by: Muge Chen <mugechen@google.com>
@foreseeable foreseeable changed the title test: refactor header inclusion to speed up building test: refactor header inclusion to speed up building (for test/common/...) Jul 14, 2020
Signed-off-by: Muge Chen <mugechen@google.com>
@junr03 junr03 self-assigned this Jul 14, 2020
@junr03
Copy link
Member

junr03 commented Jul 14, 2020

@foreseeable the failure here seems potentially real.

@@ -28,7 +29,7 @@
#include "test/integration/fake_upstream.h"
#include "test/mocks/grpc/mocks.h"
#include "test/mocks/local_info/mocks.h"
#include "test/mocks/server/mocks.h"
#include "test/mocks/server/transport_socket_factory_context.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Grpc::StatNames is an incomplete type here (https://dev.azure.com/cncf/envoy/_build/results?buildId=44924&view=logs&j=ec9f720e-f904-5f65-4ba4-f371a013344f&t=ee47c280-92b1-5c80-3a10-d3d23a58a062&l=3606); we need to #include the header providing that name. server/mocks.h probably indirectly #included it before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, server/mocks.h previously #include "common/grpc/context_impl.h" which providing that name. i have sent a pr resolve this.

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: Muge Chen <mugechen@google.com>
@junr03 junr03 merged commit 2b3d95e into envoyproxy:master Jul 18, 2020
KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020
…/...) (envoyproxy#12046)

Commit Message: refactor header inclusion to speed up building (for test/common/...)
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020
…/...) (envoyproxy#12046)

Commit Message: refactor header inclusion to speed up building (for test/common/...)
Risk Level: low
Testing: existing tests
Docs Changes: N/A
Release Notes: no
Related Issues: envoyproxy#10917

Signed-off-by: Muge Chen <mugechen@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.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