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

chore(unit-test): Add missing unit tests in modules with low coverage #1264

Merged
merged 15 commits into from
Jul 10, 2023

Conversation

eldimi
Copy link
Contributor

@eldimi eldimi commented Jul 4, 2023

Issue: #1096

Description of changes:

Add unit-tests in the following modules: core, logging, validation, parameters & serialization.
The code coverage in the logging module remains low, since some classes that were deprecated where not covered with tests in this PR.
Includes a proposal for a refactoring in the serialization module.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eldimi eldimi marked this pull request as ready for review July 4, 2023 14:58
Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

Impressive work, thank you! Maybe would have been great to split it in multiple PRs (one per module for example) but that's fine. I've added some comments here and there. The biggest thing for me is the factory stuff you've introduced in the serialization module which is too much I think.

@eldimi
Copy link
Contributor Author

eldimi commented Jul 5, 2023

Impressive work, thank you! Maybe would have been great to split it in multiple PRs (one per module for example) but that's fine. I've added some comments here and there. The biggest thing for me is the factory stuff you've introduced in the serialization module which is too much I think.

Thanks @jeromevdl Do you think I should move the refactoring to a seperate place? There is also something similar which I wanted to do in the validation module (which I didn't do because I wanted to get comments on the serialization refactoring first).
I can create a new PR with the refactoring from this one and also introduce the changes for the validation too.

@jeromevdl
Copy link
Contributor

Do you think I should move the refactoring to a seperate place? There is also something similar which I wanted to do in the validation module (which I didn't do because I wanted to get comments on the serialization refactoring first).
I can create a new PR with the refactoring from this one and also introduce the changes for the validation too.

I think you can remove it from this PR (keep it in a branch somewhere), write an issue/RFC to explain what you want to do and why (benefits and drawbacks). And according on this, we'll create a PR. I may miss something but at the moment I only see a big number of new classes. Maybe it improves testability of the module but it should not come over the size and thus speed of the library.

@eldimi
Copy link
Contributor Author

eldimi commented Jul 5, 2023

Do you think I should move the refactoring to a seperate place? There is also something similar which I wanted to do in the validation module (which I didn't do because I wanted to get comments on the serialization refactoring first).
I can create a new PR with the refactoring from this one and also introduce the changes for the validation too.

I think you can remove it from this PR (keep it in a branch somewhere), write an issue/RFC to explain what you want to do and why (benefits and drawbacks). And according on this, we'll create a PR. I may miss something but at the moment I only see a big number of new classes. Maybe it improves testability of the module but it should not come over the size and thus speed of the library.

Yes, sorry, I saw this comment first and then the one on the actual changes. I wanted to get input too here, since I too had concerns on the jar size, cold starts etc...We can do as you propose with a seperate issue and see if we need it or not.

@eldimi eldimi force-pushed the chore/unit_test_coverage branch 2 times, most recently from 8f04f31 to 88972b9 Compare July 6, 2023 11:40
Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

we are almost good, just a few imports to expand and the Bas64Gzip to change to return null when not found instead of "".

Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

One last thing and we're good.

Copy link
Contributor

@jeromevdl jeromevdl left a comment

Choose a reason for hiding this comment

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

LGTM !

@jeromevdl jeromevdl merged commit c7aedc4 into aws-powertools:main Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants