Skip to content

dfp: adding connection timing test#19766

Merged
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:timing_testing
Feb 2, 2022
Merged

dfp: adding connection timing test#19766
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:timing_testing

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Looks good!

"\n", cache_file_value_contents_));
}
}
void testConnectionTiming(IntegrationStreamDecoderPtr& response, bool cached_dns,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: newline before?

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.

done

const std::string& override_auto_sni_header = "") {
setUpstreamProtocol(Http::CodecType::HTTP1);
const std::string filename = TestEnvironment::temporaryPath("dns_cache.txt");
::unlink(filename.c_str());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, with your new O_TRUNC writes, is this still required?

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.

I think so - not all the tests write the file and the file shouldnt exist in the tests which don't use one

ASSERT_LE(handshake_end, request_send_end);
ASSERT_LE(request_send_end, response_begin);
ASSERT_LT(handshake_end, timeSystem().monotonicTime().time_since_epoch().count());
ASSERT_EQ(now, time.time_since_epoch().count());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this guaranteed to be true since some time will have elapsed between line 182 and here?

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.

oh sorry, that was me testing my understanding of monotonic time when I was debugging out of order times. Because it's latched, it shouldn't change as time passes. But now that I've verified I understand it I can remove that.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk merged commit fe5d87c into envoyproxy:main Feb 2, 2022
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
Part of envoyproxy/envoy-mobile#1520

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Josh Perry <josh.perry@mx.com>
@alyssawilk alyssawilk deleted the timing_testing branch August 4, 2022 01:09
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.

2 participants