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

Refactor interface tests and add more #279

Merged
merged 4 commits into from
Oct 5, 2020

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Oct 1, 2020

Fixes: # -

Description of the changes being introduced by the pull request:

Note, this PR does not change the interface behavior, it only cleans up the test module, with the goal to make future interface changes more easily comprehensible by looking at changes in the test module.

The clean up includes:

  • Remove repetitive test dir creation in individual test functions.
    Use setUp function instead
  • Remove redundant test dir creation in setUpClass function.
  • Remove repetitive key generation and key import code used to test
    one or the other "unit".
    Instead aggregate key generation and loading testing in a single
    test function per key type (rsa, ed25519, ecdsa).
  • Use context manager for assertRaises to increase readability
  • Use test tables (for loops) for more extensive testing of
    different combinations of args and kwargs.

This PR also includes additional testing:

  • interactive password prompts tests (rendering coverage exemption
    pragmas obsolete).
  • import regression tests for ed25519 and ecdsa keys, using
    pre-generated keys instead of on-the-fly generated keys.
  • stronger assertions, e.g. actually check the key length to
    test the custom bits parameter for RSA key creation, or look at
    error message contents
  • more error testing, e.g. by trying to load invalid keys, or keys

This PR also identifies and adds tests and FIXME comments for unexpected interface behavior, identified while updating the tests, i.e.

  • import_ed25519_publickey_from_file can import private keys
  • import_ed25519_privatekey_from_file can import public keys
  • import_ecdsa_publickey_from_file can import ed25519 public keys

Please verify and check that the pull request fulfils the following requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

ed25519 and ecdsa key pairs creation script:

```
import securesystemslib.interface as i
i.generate_and_write_ed25519_keypair(
    "tests/data/keystore/ed25519_key", password="password")
i.generate_and_write_ecdsa_keypair(
    "tests/data/keystore/ecdsa_key", password="password")
```

Also adds json file for custom json key format parsing error
testing.
- Remove repetitive test dir creation in individual test functions.
  Use setUp function instead
- Remove redundant test dir creation in setUpClass function.
- Remove repetitive key generation and key import code used to test
  one or the other "unit".
  Instead aggregate key generation and loading testing in a single
  test function per key type (rsa, ed25519, ecdsa).
- Use context manager for assertRaises to increase readability
- Use test tables (for loops) for more extensive testing of
  different combinations of args and kwargs.

Additional testing includes:
- interactive password prompts tests (rendering coverage exemption
  pragmas obsolete).
- import regression tests for ed25519 and ecdsa keys, using
  pre-generated keys instead of on-the-fly generated keys.
- stronger assertions, e.g. actually check the key length to
  test the custom bits parameter for RSA key creation, or look at
  error message contents
- more error testing, e.g. by trying to load invalid keys, or keys

Additionally, identifies and adds tests + FIXME comments for
unexpected behavior of the interface:
- import_ed25519_publickey_from_file can import private keys
- import_ed25519_privatekey_from_file can import public keys
- import_ecdsa_publickey_from_file can import ed25519 public keys
Recent test additions these test coverage exemptions obsolete.
@coveralls
Copy link

coveralls commented Oct 1, 2020

Coverage Status

Coverage decreased (-0.08%) to 98.878% when pulling a04b386 on lukpueh:refactor-interface-tests into 8b311bf on secure-systems-lab:master.

@SantiagoTorres
Copy link
Collaborator

Wow, we lost all our coverage. I assume this is a misconfig or a bug...

@trishankatdatadog
Copy link
Contributor

Coverage is honestly one of the well-intentioned but mostly-misguided things I've seen in testing...

@lukpueh
Copy link
Member Author

lukpueh commented Oct 2, 2020

Wow, we lost all our coverage. I assume this is a misconfig or a bug...

I think that was caused by a force-push interrupting a build.

Coverage is honestly one of the well-intentioned but mostly-misguided things I've seen in testing...

I think it's not that bad. :) But I agree that one should not overrate coverage results, especially if developers are very liberal with coverage exemptions, some of which I'm removing in this PR.

Btw. the build is still failing because of coverage dropping below 99% on Python 3.5. Looking at the results, it seems like it's only a rounding difference, because the absolute coverage is the same as on other versions. Anyways, I think 99% is a rather strict threshold. If you don't mind I'll lower it a bit.

Our coverage is currently very close to the strict 99 threshold,
and we sometimes go below due to different rounding strategies on
different builds (different coverage versions?) or if we add/remove
code and the relative coverage ratio changes slightly.

This commit lowers the strict coverage threshold from 99 to 97.

are very close to that threshold
Copy link
Collaborator

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

Looks like a solid set of enhancements to me, thanks Lukas!

Could you file the FIXME comments as issues?

@lukpueh
Copy link
Member Author

lukpueh commented Oct 5, 2020

Thanks for the review, @joshuagl!

Could you file the FIXME comments as issues?

Done in #280.

Merging...

@lukpueh lukpueh merged commit 0ec854b into secure-systems-lab:master Oct 5, 2020
@lukpueh lukpueh deleted the refactor-interface-tests branch May 25, 2023 10:41
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.

5 participants