services/tunnelbroker/test/CryptoToolsTest.cpp
13–36 ↗	(On Diff #11206)	It's usually a good practice to have only one assertion in a test. The benefit is that by looking at tests result you exactly know which assertions were violated. Also, when a test has multiple assertion, the execution stops on the first failure and we don't know what would be the result of following assertions. So please split this test into two.

This revision now requires changes to proceed.Apr 8 2022, 12:39 AM

Split the test into two.

• max marked an inline comment as done.Apr 11 2022, 5:24 AM

• max added inline comments.

services/tunnelbroker/test/CryptoToolsTest.cpp
13–36 ↗	(On Diff #11206)	It's usually a good practice to have only one assertion in a test. The benefit is that by looking at tests result you exactly know which assertions were violated. Also, when a test has multiple assertion, the execution stops on the first failure and we don't know what would be the result of following assertions. So please split this test into two. That makes sense to me. Split it into two.

tomek added inline comments.

services/tunnelbroker/test/CryptoToolsTest.cpp
13 ↗	(On Diff #11277)	We should be able to use simple tests https://google.github.io/googletest/primer.html#simple-tests instead of fixtures https://google.github.io/googletest/primer.html#same-data-multiple-tests. `TEST` is used when you don't need to share data between tests and `TEST_F` when you need that.

This revision is now accepted and ready to land.Apr 11 2022, 5:50 AM

• max marked 2 inline comments as done.Apr 18 2022, 3:19 AM

• max added inline comments.

services/tunnelbroker/test/CryptoToolsTest.cpp
13 ↗	(On Diff #11277)	We should be able to use simple tests https://google.github.io/googletest/primer.html#simple-tests instead of fixtures https://google.github.io/googletest/primer.html#same-data-multiple-tests. `TEST` is used when you don't need to share data between tests and `TEST_F` when you need that. Good catch! Thanks, @palys-swm.