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

Fix Secure Cell corruption tests on i386 #408

Merged
merged 2 commits into from
Mar 4, 2019
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Mar 4, 2019

It turned out that current tests do not work on i386 architecture (32-bit x86). This is because we try allocating ~0xDEADBEEF bytes which is not possible in 32-bit userland that typically has a limit of 2 or 3 GB of user memory. The constant we use does not really matter but should be distinct enough. Use something smaller instead.

It turned out that current tests do not work on i386 architecture
(32-bit x86). This is because we try allocating ~0xDEADBEEF bytes
which is not possible in 32-bit userland that typically has a limit
of 2 or 3 GB of user memory. The constant we use does not really
matter but should be distinct enough. Use something smaller instead.
@ilammy ilammy added the tests Themis test suite label Mar 4, 2019
@ilammy ilammy requested a review from shadinua March 4, 2019 11:26
@Lagovas
Copy link
Collaborator

Lagovas commented Mar 4, 2019

copy-paste 6 times one value not good. what about to declare it as #define (to change in one place next time : ) and add a comment why we chose this value

@ilammy
Copy link
Collaborator Author

ilammy commented Mar 4, 2019

It's not the 'same' value so I think we don't need to have a named constant for it. It just happens to be the same in all these places because I'm too lazy to invent different values for each test, but they work fine with different values. These are really arbitrary values (well, as long as they are less that 231).

My preference for tests is to keep individual tests independent of each other, allow some redundancy and avoid abstraction unless it improves readability, especially for test inputs and outputs. For example, when testing encryption I'd rather see "message" directly as plaintext rather than some plaintext constant of some type defined elsewhere. However, if we need to use the same value in several tests then it makes sense to define it as a constant. Similarly, encryption keys are big and it's more readable to use constants rather than insert them directly.

That's why I'm not cringing at what looks like blatant violation of DRY principle here. I don't remember where exactly I picked this up, but this post summarizes its nicely.

@Lagovas
Copy link
Collaborator

Lagovas commented Mar 4, 2019

my suggest to use one #define has one more goal: avoid using magic numbers like 0xDEADBEEF/0x1337BEEF because everytime I see them I ask myself: "why these values, not 0xAAAAAAAA or 0xDDDDDDDD?
Tests don't depends on some specific values. So no matter to use same value or different in each test. So why not to use one with some readable name such some_big_32bit_value_for_alloc or add some comment for constant that will explain that this constant should be used in context where we check allocation with big memory requirement? or abstract it in some method like get_random_int which will explain that here we don't need any specific value

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

thx

@vixentael vixentael added the core Themis Core written in C, its packages label Mar 4, 2019
@ilammy ilammy merged commit 9fcc423 into master Mar 4, 2019
@ilammy ilammy deleted the ilammy/i386-crashes branch July 26, 2019 13:58
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 tests Themis test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants