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

cherry-pick from release/0.15.0 to master. Part 3. Python Unittest #1057

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

radetsky
Copy link
Contributor

@radetsky radetsky commented Sep 5, 2024

Add python unittests that check the compatibility with bytes generated on iOS 17, but OpenSSL 1.1 (#1021)

Checklist

  • Change is covered by automated tests
  • The [coding guidelines] are followed

@vixentael
Copy link
Contributor

@Lagovas JFYI new pythemis test

@Lagovas
Copy link
Collaborator

Lagovas commented Sep 6, 2024

I don't understand why we need one more test? they test any new case or flow? or it's regression test for some incident? then will be great to see what incident caused problems in decryption data from ios app in python code

@Lagovas
Copy link
Collaborator

Lagovas commented Sep 6, 2024

in #1021 I see that was added tests but also don't see why we did it and for what

@vixentael
Copy link
Contributor

that's just an additional test in our suite, it's a good thing

@Lagovas
Copy link
Collaborator

Lagovas commented Sep 6, 2024

that's just an additional test in our suite, it's a good thing

okay, but what prompted you to add this test?
we already have cross-platform integration tests that encrypt/decrypts data using library from different languages and different OS, and python tested too.

maybe we need extend these tests instead of adding edge case without any reason to add it. it's clear for what if we had incident and it will be regression tests
because now this test hard to maintain (need simulator with specific version and specific openssl) and without knowledge how long we are interested in this test. does it test environment features or themis implementation.

@radetsky
Copy link
Contributor Author

As we discuss the test, iOS 18 is already out. I don't mind forgetting about this test because it is no longer relevant.
There is already a newer version of iOS, and we need to think about making Themis work in it.

Finally, what should I do with the pull request?

@Lagovas
Copy link
Collaborator

Lagovas commented Sep 27, 2024

Let me explain why I don't like tests like that. These tests looks like we have function

def themis_func(x, y):
  return math_lib.sum(x, y)

We have a themis function that can use some math lib (openssl, boringssl) and returns result. This function can be compiled by different compilers and used by a lot of languages. And we calculated one result on one of the platform and test that it similar on the another platform.

And it looks like we calculated 2+2 on ios15 and check that 2+2 the same on GH runner.
2+2 works similar on all platforms. We only need to check that all the code compiled and started together, math_lib properly imported into the themis, it produced some valid output in the real environment and we compare it with the expected value. We don't test here the algorithm used to calculate because he is similar everywhere.

And this test doesn't test anything related to ios15 instead of it was compiled on one of the real machine and stored. And no one can reproduce it again except Rad, who can probably change own env after the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants