lan_discovery_test and version_test cleanup #967
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/version_test.c, line 13 at r1 (raw file):
TOX_VERSION_IS_API_COMPATIBLE(major, minor, patch), argument) #define FUZZ_VERSION \
This is not good.
This is adding logic to the tests, making them more complicated with no value added. The current test showcases exactly the situations we want to test. The changed tests need more thought to understand. Also, the macro you added here is a poorly behaved one. We will only allow well-behaved macros, i.e. ones that behave mostly like functions. This macro is a syntax extension (something like a new control structure) rather than an abbreviation of code.
auto_tests/version_test.c, line 23 at r1 (raw file):
bool actual, bool expected) { ck_assert_msg(actual == expected,
This is good. Let's keep this change, but not the fuzz bit.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/version_test.c, line 24 at r1 (raw file):
{ ck_assert_msg(actual == expected, "Client version %d.%d.%d is %s compatible with library version %d.%d.%d, but it should %s be.\n",
There was a reason for this. Your change made it so that it says "... is compatible ..." (note the double space). Please revert that.
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/version_test.c, line 24 at r1 (raw file):
Previously, iphydf wrote…
There was a reason for this. Your change made it so that it says
"... is compatible ..."(note the double space). Please revert that.
Turns out, markdown doesn't show the two spaces.
"... is compatible ..."
^^ two spaces here
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/version_test.c, line 13 at r1 (raw file):
Previously, iphydf wrote…
This is not good.
This is adding logic to the tests, making them more complicated with no value added. The current test showcases exactly the situations we want to test. The changed tests need more thought to understand. Also, the macro you added here is a poorly behaved one. We will only allow well-behaved macros, i.e. ones that behave mostly like functions. This macro is a syntax extension (something like a new control structure) rather than an abbreviation of code.
I actually did this to make them more readable. Currently the tests are a bunch of unexplained specific comparisons. Now the test_api function is evaluated based on whether or not the output matches expected conditions, and under more inputs. I can see how the macro extension is a terrible idea though.
I'll send a new commit in attempting to revert the file back to its old method, but I'll be a dunce and add some comments for each section showcasing exactly what they're checking.
auto_tests/version_test.c, line 24 at r1 (raw file):
Previously, iphydf wrote…
Turns out, markdown doesn't show the two spaces.
"... is compatible ..." ^^ two spaces here
Srry
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained
auto_tests/version_test.c, line 65 at r2 (raw file):
#define TOX_VERSION_MINOR 0 #define TOX_VERSION_PATCH 4 //Beyond 0.*.* Tox is comfortable with any lower version within their major
Space after //.
complete! 1 of 1 LGTMs obtained
|
Squash all commits into one. |
|
What did I do... Edit: |
|
Yes, but there is still a merge commit in there. |
|
Fuck I I cant do it |
|
I'll do it. |
|
I'm so sorry you have to deal with the shit from people like me |
Removed a pointless declaration of a function in lan_discovery_test and cleaned up the one error message there. Did an entire restructuring of the version_test using macros that resulted in fewer lines of code but more thorough testing. Formatting of version_test.c back to old way, save comments and one change Missing space My greatest enemy Add `#include <cstdio>` for `std::printf`. Make tox.c unambiguously parseable. Rules: 1. Constants are uppercase names: THE_CONSTANT. 2. SUE[1] types start with an uppercase letter and have at least one lowercase letter in it: The_Type, THE_Type. 3. Function types end in "_cb": tox_friend_connection_cb. 4. Variable and function names are all lowercase: the_function. This makes it easier for humans reading the code to determine what an identifier means. I'm not convinced by the enum type name change, but I don't know a better rule. Currently, a lot of enum types are spelled like constants, which is confusing. [1] struct/union/enum Use run_auto_test.h test fixture for some auto-tests. Most of the auto-tests should use this fixture, but I've only done a few to set an example.
|
No worries, thanks for the contribution :). |
iphydf
changed the title
Another minor unit test PR. Big one coming up soon.
Removed a pointless declaration of a function in lan_discovery_test
and cleaned up the one error message there. Did an entire restructuring
of the version_test using macros that resulted in fewer lines of code but more
thorough testing.
This change is