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

Tests refactored code: reorganize testing in test_api #1414

Closed
4 tasks
MVrachev opened this issue May 21, 2021 · 5 comments
Closed
4 tasks

Tests refactored code: reorganize testing in test_api #1414

MVrachev opened this issue May 21, 2021 · 5 comments
Labels
backlog Issues to address with priority for current development goals testing

Comments

@MVrachev
Copy link
Collaborator

MVrachev commented May 21, 2021

Description of issue or feature request:
Currently, there are too many things tested in tuf/test_apy.py including:

There are multiple occurrences wherein a single test function we have the first two tested
and additional specific testing added.

I propose to:

  1. Create a new testing folder that will contain test modules for the refactored code. Named for example test_refactor
  2. Split test_api into multiple logical test modules.
    For example: test_serialization_deserialization (I can't think of an alternative name...) and test_metadata_api (which will contain only API specific to the metadata classes without (de)serialization tests),
  3. Move those new test modules into the new folder.
  4. Make sure we can run all tests in the new test folder together with the other tests or in other words make the necessary changes in tests/aggregate_tests.py.

The reason why I propose splitting test_api is that it's bloated and hard to navigate through.

Additionally, I think that a separate folder for the refactored code will be useful long term considering we are going to add more and more refactored code that should be tested.
Then, one day when we fully replace the old code with the refactored code we can just remove all of the old tests and move the
content of this new folder into the tests folder.

@jku
Copy link
Member

jku commented May 25, 2021

Split test_api into multiple logical test modules.

Yes, agreed on this.

I'm not sure new directories are needed (especially if you're just splitting one file into two) but I can live with it.

The alternative to splitting into two files (serialization and everything else) is splitting by class: I think that would be worth considering as it seems logical and easy to learn: if I want to look at Key tests I look in test_api_key.py (or whatever the name is). This could be done piece by piece: We could just move the largest classes out of test_api.py now, and see how it evolves

@MVrachev
Copy link
Collaborator Author

Splitting test_api.py into modules containing class-specific tests seems reasonable and logical.
On another hand, this will make some of the tests harder like #1382 and #1391.
For #1391, I am preparing a pr, which adds a single module that focuses on (de)serialization tests.
There, it's useful I can reuse some of the targets valid data I have defined when testing DelegationRole, Delegation, and TargetFile.
We are going to discuss this approach in the pr, I just wanted to mention it here.
Before moving on with this issue, I will make sure to push the pr for #1382.

@jku
Copy link
Member

jku commented May 25, 2021

Yes, the test data definition/generation is an issue: something I'd consider is defining only the data for the "class under test" explicitly and generating test data for any child objects (because they have their own tests and this might allow defining the test data in reasonable space)

So something like

def child() -> str
     # return a valid serialized ChildObject
     return metadata.ChildObject(1,2,3).to_string()

valid_parent_instances={
    "empty foo list": f"{'foo':[], 'child_object': {child()}}"
    "non-empty foo list": f"{'foo':['item'], 'child_object': {child()}}"
    "no child": f"{'foo':['item'], 'child_object': None}"
}

there is no to_string() currently but you get the idea: we're not trying to test ChildObject validity here but just create a valid one so we can test the parent ... It's totally possible that this does not scale to the complex objects we have: I'm just writing down things I'd test.

@MVrachev MVrachev added the backlog Issues to address with priority for current development goals label May 26, 2021
@MVrachev
Copy link
Collaborator Author

I am not sure if there is a point in separating test_api.py into multiple test modules right now.
The file right now is closer to 600 lines than 1000 as it was before.
After moving all serialization tests in a separate module from test_api.py into test_metadata_serialization.py in #1416,
test_api.py became a lot simpler.
Additionally, I am suggesting two additional changes simplifying this file #1541 and 23af38a.

@MVrachev
Copy link
Collaborator Author

MVrachev commented Sep 1, 2021

I think the original proposition to split test_api into multiple modules is no longer necessary.
We moved the serialization tests and all of the tests in test_api are truly related to the API.
test_api can be simplified and this could be discussed, but this issue and considering its original purpose is not
the place to do that. If needed a new issue can be created.

@MVrachev MVrachev closed this as completed Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals testing
Projects
None yet
Development

No branches or pull requests

3 participants