Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce C formatting in Soter and Themis tests #399

Merged
merged 2 commits into from
Mar 2, 2019

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Feb 28, 2019

Reformat the tests in accordance with the code style.

The code looks unreadable in some places, but meh... We use way too many long_and_detailed_multi_word_identifiers as well as test macros which effectively require to write code in a single line.

I'd suggest us to just deal with it (⌐■ _■) Let's leave this code as is now. For new code we can make an effort to keep it readable with the new formatting rules. Older code can be refactored to be more readable when we actually change it.

@ilammy ilammy added core Themis Core written in C, its packages infrastructure Automated building and packaging tests Themis test suite labels Feb 28, 2019
@vixentael
Copy link
Contributor

Uh oh, it's really complicated to read

    testsuite_fail_unless(SOTER_INVALID_PARAMETER ==
                              soter_asym_cipher_init(&ctx, key_data, key_data_length,
                                                     (soter_asym_cipher_padding_t)0xffffffff),
                          "soter_asym_cipher_init: invalid algorithm type");

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

changes-changes everywhere 🤔

Makefile Outdated
@@ -340,6 +340,25 @@ $(TEST_OBJ_PATH)/%.opp: $(TEST_SRC_PATH)/%.cpp
@echo -n "compile "
@$(BUILD_CMD)

FMT_FIXUP += $(THEMIS_TEST_FMT_FIXUP) $(SOTER_TEST_FMT_FIXUP)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. is it only for tests? shall we name it FMT_TESTS_SETUP?
  2. is it ok to have these in main makefiles, maybe move to tests/test.mk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vixentael,

  1. is it only for tests? shall we name it FMT_TESTS_SETUP?

FMT_FIXUP is the variable which gathers all files to be formatted. We can introduce an intermediate variable to keep THEMIS_TEST_FMT_FIXUP and SOTER_TEST_FMT_FIXUP but it won't increase readability so I think it's redundant.

  1. is it ok to have these in main makefiles, maybe move to tests/test.mk?

I have placed these lines in the main makefile because that's where compilation rules for test binaries are located. However, it really makes sense to put them into tests/test.mk. How about moving both new formatting rules and existing build rules for tests there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vixentael I've moved the formatting setup as well as compilation to tests/test.mk.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks! my inner perfectionist feels much better now. Also naming FMT_FIXUP has more sense now when places into tests.mk

@ilammy
Copy link
Collaborator Author

ilammy commented Mar 1, 2019

@vixentael

Uh oh, it's really complicated to read

This should be easier to understand but it requires a bit more changes and cannot be automated:

    result = soter_asym_cipher_init(&ctx, key_data, key_data_length, PADDING);
    testsuite_fail_unless(result == SOTER_INVALID_PARAMETER,
                          "soter_asym_cipher_init: invalid algorithm type");

@vixentael
Copy link
Contributor

it requires a bit more changes and cannot be automated

ok-ok, i'm just complaining

@vixentael vixentael self-requested a review March 1, 2019 13:16
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

lgtm
(but complicated to read :D)

@ilammy
Copy link
Collaborator Author

ilammy commented Mar 1, 2019

it requires a bit more changes and cannot be automated

ok-ok, i'm just complaining

I'm very much triggered by the readability as well so I guess I'll be slowly fixing it up function by function when I need to actually change the files. It's not like I experience a physical pain in the eyes by looking at it, but the moral suffering from not being able to configure the formatter for beau-ti-ful code and having to look at this instead is still there.

Or maybe some day I'll discover that magical set of clang-format keys which makes the code readable out of the box (and we'll see yet another formatting-only commits).

ilammy and others added 2 commits March 2, 2019 22:40
Add Soter and Themis tests to the formatting loop. Use the same
approach as for the main source code.
The code looks unreadable in some places, but meh... We use way too
many long_and_detailed_multi_word_identifiers as well as test macros
which effectively require to write code in a single line.
@ilammy ilammy merged commit 85aa09b into cossacklabs:master Mar 2, 2019
@ilammy
Copy link
Collaborator Author

ilammy commented Mar 2, 2019

The checks did not pass because of a failure in Rust tests (again), but I've merged this PR regardless. I've seen this failure before and have a hunch about why it happens. Though, no solution yet.

@ilammy ilammy deleted the formatting-tests branch March 28, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages infrastructure Automated building and packaging tests Themis test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants