Skip to content

Use TestEnvironment::nullDevicePath instead of /dev/null/#12172

Merged
mattklein123 merged 11 commits intoenvoyproxy:masterfrom
davinci26:rtds_integration_test_config_path
Aug 3, 2020
Merged

Use TestEnvironment::nullDevicePath instead of /dev/null/#12172
mattklein123 merged 11 commits intoenvoyproxy:masterfrom
davinci26:rtds_integration_test_config_path

Conversation

@davinci26
Copy link
Copy Markdown
Member

@davinci26 davinci26 commented Jul 18, 2020 •
edited
Loading

Signed-off-by: davinci26 sotirisnan@gmail.com
Changes the config file to use the cross-platform TestEnvironment::nullDevicePath() instead of /dev/null in:

  1. rtds_integration_test.cc
  2. cluster_integration_test.cc
  3. redis_cluster_integration_test.cc
  4. mysql_integration_test.cc
  5. postgres_integration_test.cc
  6. rtds_integration_test.cc

Additional Description:

The test list is flaky on Windows. I think it is related to #11793 and it will be less flaky when this change gets merged.

Risk Level: N/A
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

davinci26 added 2 commits July 18, 2020 15:25
@davinci26
Change /dev/null to TestEnvironment::nullDevicePath()
0348ae1 
Signed-off-by: davinci26 <sotirisnan@gmail.com>
@davinci26
removed whitespace change
eb9070a 
Signed-off-by: davinci26 <sotirisnan@gmail.com>
@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Jul 18, 2020

@nigriMSFT @wrowe @sunjayBhatia

@dio
Copy link
Copy Markdown
Member

dio commented Jul 19, 2020

@davinci26 seems like rtds_integration_test is still tagged as fails_on_windows? Probably remove that tag to test that on CI? Thank you!

@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Jul 19, 2020

@dio I added why I didn't remove the tag in the pr description. The test list is still flaky. I assume my other pr will help.

@davinci26 davinci26 mentioned this pull request Jul 23, 2020
Merged
@davinci26 davinci26 requested a review from snowp as a code owner July 24, 2020 00:01
@davinci26 davinci26 changed the title Makes access_log_path in Rtds integration cross platform Use TestEnvironment::nullDevicePath instead of /dev/null/ Jul 24, 2020
@davinci26 davinci26 requested a review from mattklein123 as a code owner July 24, 2020 00:48
@davinci26
fix cluster_integration_test.cc && redis_cluster_integration_test.cc
135bd76 
Signed-off-by: davinci26 <sotirisnan@gmail.com>
@davinci26 davinci26 force-pushed the rtds_integration_test_config_path branch from 0355a7f to 135bd76 Compare July 24, 2020 00:50
@davinci26
more dev/null
e20c898 
Signed-off-by: davinci26 <sotirisnan@gmail.com>
@davinci26 davinci26 requested a review from dio as a code owner July 24, 2020 19:49
@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Jul 24, 2020

@sunjayBhatia do you think we could write a format rule for this one as well?

@davinci26
format changes
d580563 
Signed-off-by: davinci26 <sotirisnan@gmail.com>
@sunjayBhatia
Copy link
Copy Markdown
Member

sunjayBhatia commented Jul 24, 2020 •
edited
Loading

@sunjayBhatia do you think we could write a format rule for this one as well?

yeah I think that is a good idea, especially as test cases get copied and pasted, if there are any /dev/nulls floating around, they will keep coming back

maybe something as simple as searching for /dev/null in a line or even more strict access_log_path: /dev/null? or maybe regex like .*: "?/dev/null"? might be worth playing around with, theres some code/examples in the format checker that applies certain checks on certain directories or not, I forget if theres format checks already on test fixture files

envoy/tools/code_format/check_format.py

Line 580 in 2a34c76

def checkSourceLine(line, file_path, reportError):
for examples and tests:

envoy/tools/code_format/check_format_test_helper.py

Line 149 in 2a34c76

def runChecks():

@davinci26
formatting
792ddc8 
Signed-off-by: davinci26 <sotirisnan@gmail.com>
@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Jul 25, 2020

I will fix the CI (formatting) as I add the new clang-ci rule

@wrowe
Copy link
Copy Markdown
Contributor

wrowe commented Jul 30, 2020

Ping? Looking forward to this merge after the format failures are fixed.

@wrowe wrowe mentioned this pull request Jul 30, 2020
Merged
davinci26 and others added 2 commits July 30, 2020 13:21
@davinci26
ws changes
7cedce8 
Signed-off-by: davinci26 <sotirisnan@gmail.com>
more format changes
7e90e37 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
wrowe
wrowe previously approved these changes Jul 31, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

LGTM

dio
dio reviewed Jul 31, 2020
View reviewed changes
test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc
op_timeout: 5s
)EOF";
)EOF",
TestEnvironment::nullDevicePath());
Copy link
Copy Markdown
Member

@dio dio Jul 31, 2020

Choose a reason for hiding this comment

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

This one is too far?

Format check failed, try apply following patch to fix:

diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc
index b67cab241..928758cd7 100644
--- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc
+++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc
@@ -60,7 +60,7 @@ static_resources:
           settings:
             op_timeout: 5s
 )EOF",
-                                                        TestEnvironment::nullDevicePath());
+                                       TestEnvironment::nullDevicePath());

// This is a configuration with command stats enabled.
const std::string CONFIG_WITH_COMMAND_STATS = CONFIG + R"EOF(

Copy link
Copy Markdown
Member Author

@davinci26 davinci26 Jul 31, 2020

Choose a reason for hiding this comment

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

I am getting really confused... I am pretty sure I had it like this in the previous commit. Look at the diff here 7cedce8

ws part 100
25428a3 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26 davinci26 force-pushed the rtds_integration_test_config_path branch from a68cbbe to 25428a3 Compare July 31, 2020 16:21
Sotiris Nanopoulos added 2 commits July 31, 2020 11:43
format ws++
8676f81 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
escape fmt chars
daba048 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Jul 31, 2020

It took me more time that I am proud to admit but I finally got there. The CI is clean now.

Let me know if you have any other comments 😄

dio
dio approved these changes Aug 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks

@mattklein123 mattklein123 merged commit e13b690 into envoyproxy:master Aug 3, 2020
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
test: use TestEnvironment::nullDevicePath instead of /dev/null/ (envo…
fccad39 
…yproxy#12172)

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
test: use TestEnvironment::nullDevicePath instead of /dev/null/ (envo…
0245376 
…yproxy#12172)

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@snowp snowp Awaiting requested review from snowp

@mattklein123 mattklein123 Awaiting requested review from mattklein123

2 more reviewers

@dio dio dio approved these changes

@wrowe wrowe wrowe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wrowe wrowe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@davinci26 @dio @sunjayBhatia @wrowe @mattklein123