Tools - is an internal toolset of functions.
We perform tests on such functions:
- generateRandomString(),
- validateDeviceID(),
- validateSessionID().
Details
Details
- Reviewers
• karol tomek varun - Commits
- rCOMM74b9fb6cdb02: [services] Tunnelbroker - Tools tests
Run yarn test-tunnelbroker-service all tests are run and succeed.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- tunnelbroker-tests-tools
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) | We probably shouldn't use randomly generated data for tests. |
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) | Yeah, definitely. The issue with randomly generated data is that repeated test runs might have a different outcome (either success or failure depending on the randomness). If we want to use random data, we should generate it and hardcode in the test. |
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) |
I don't agree with you guys here. If the function doesn't return consistent results during the randomly generated data (using the expected pattern to generate) something wrong with the realization. The only reason not to use the generated data is that test is not repeatable anymore. But the pros of using the generated data are that we test the wide range of input data. The "monkey test" can smoke out the bugs from the "dark corners" of the implementation. The folly library (and I think FB too) use random data in the tests. Please take a look at: FBStringTest and ConcurrentSkipListTest for example. @ashoat can you comment your thoughts here too about FB practice? |
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) | So let's suppose your random test failed. Then you changed something and the test succeeded. How would you know if you actually fixed the issue or were just lucky? How many times would you plan to run the test to make sure that it passes? Agree, random tests are sometimes useful, but it's a lazy approach to let them find corner cases instead of creating a lot of test cases checking each of them. At least random tests should be an addition to a solid set of tests exploring all the corner cases. |
Comment Actions
I'm not blanket opposed to tests that generate random data at runtime, but I do think it's always better (if you have time) to break it down into multiple tests that use a representative set of random data that was generated when the test was written. Overall I don't think this matters a whole lot, though – I worry about us spending too much time on tests as opposed to feature / product work.
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) | I think the main purpose of these tests is to assure that new changes don't break anything. With tests using random data, you can never be sure about this. You can always generate random data and hardcode it in the tests. If you hardcode multiple data sets, you'll also check the consistency of the tests' results. Another story would be to introduce something like a fuzzer, but I don't think we need it that much. |
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) |
I've added the generated value printing if the test fails. Now the failed test can be reproducible. The main point of tests in addition to what has already been mentioned is to find code defects before it goes to the user. Of course, we can not predict all the cases, but we can test the wide range of the input values at a low cost using the generated values. With the addition of printing the generated value, these tests now are:
If we don't have a generated values we can face called "Pesticide paradox":
I'm flexible with this and we can use the "mix" of generated and static values in the tests, as we agreed yesterday at the meeting. It's a good point to print the generated values if we have randomly generated values in the tests. This is a good catch! Also, we can discuss this tomorrow during Office hours if we want to make some rules for it. |
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) |
I added my comment at 1052 and you added yours at 1108 so I couldn't see it. Maybe you forgot to hit "Submit" button - IDK. You never responded to this:
We can do a mix, static + generated data as you said:
But I cannot see any static data here(or maybe I missed it?). In all other services we use static data, we should have consistent tests in all services. |
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 19–22 ↗ | (On Diff #11187) |
I think we have some race conditions here )) I am writing while you are responding )
I'm of course agree with the first. But if you are skipping the random tests keep in mind that the random data can break your things - something wrong with it. You should consider validating that data, etc. to prevent this (called the "Monkey test"). We are generating data by the expected pattern. That's why if the expected generated data break things - it's something wrong with the implementation. I am convinced that implementation needs to be tested with a wider range of values if it's "cheap" in the realization and work cost.
It doesn't make sense to use static data along with the random generated in one test. Because if the expected generated pattern data break the things - something is wrong here. In real-world usage, there will be no static data passed to it.
I had a question yesterday at our sync - do we need some rules for this to follow? I thought that the solution was - no, no certain rules. So we can use a mix if the test does not take too long to implement. We should consider using better approaches if we can and be flexible or use some rules for the team. The main concern about using it was that it was not reproducible. Now it is because we will know the values if it fails. In addition, we will have wider value range testing with no additional cost. This is a widespread practice to use random pattern-generated data in many big projects. I've mentioned folly before as well. |
Comment Actions
It looks like there's a strong disagreement here. We can easily satisfy both approaches by adding some tests with static test data - hardcoded strings. Would you guys @geekbrother @karol-bisztyga agree with that?
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 15–17 ↗ | (On Diff #11337) | It looks like they provide operator<< for us - nice find! |
Comment Actions
Added tests with static variables along with generated ones. Switched from TEST_F to TEST, because we don't use fixtures here.
Comment Actions
Ok, let's be flexible on it!
I've added tests with the static variables along with the generated ones.
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 15–17 ↗ | (On Diff #11337) |
Thanks! |
Comment Actions
In this diff naming scheme is really strange. This scheme made some sense in previous diffs, but here I can't find any. As an example of improved name, you can replace TestOperationsOnValidateSessionIDInvalid with ValidateSessionShouldReturnFalseForInvalidSession. We can also consider creating a separate test class ValidateSessionIDTests that has a test ShouldReturnFalseForInvalidSession.
Comment Actions
Some high-level feedback for the team here: we are spending a lot of time on tests. It's hard to justify this from a product priority perspective. I think we need to find a way to spend less time writing tests, or alternately to deprioritize automated testing entirely if we aren't able to more immediately reduce the cost of writing tests.
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 20 ↗ | (On Diff #11698) | Please fix this in all the names |
| services/tunnelbroker/test/ToolsTest.cpp | ||
|---|---|---|
| 20 ↗ | (On Diff #11698) |
Thanks. Fixed. |