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: split controllers to separate packages + cover them with unit tests #404

Merged
merged 9 commits into from
Mar 27, 2023

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Mar 17, 2023

This PR

  • moves controllers to separate packages
  • moves common functions/constants into separate common package
  • migrates ginkgo controller tests to basic unit tests

Related Issues

Fixes #393

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #404 (155c5bb) into main (6133060) will increase coverage by 7.03%.
The diff coverage is 68.88%.

@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
+ Coverage   71.09%   78.12%   +7.03%     
==========================================
  Files          11       12       +1     
  Lines         851      855       +4     
==========================================
+ Hits          605      668      +63     
+ Misses        217      153      -64     
- Partials       29       34       +5     
Impacted Files Coverage Δ
...ollers/core/featureflagconfiguration/controller.go 54.63% <35.29%> (ø)
...rollers/core/flagsourceconfiguration/controller.go 45.16% <62.50%> (ø)
controllers/common/common.go 100.00% <100.00%> (ø)
Flag Coverage Δ
component-tests 80.28% <ø> (+15.48%) ⬆️
unit-tests 36.25% <68.88%> (+11.69%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@odubajDT odubajDT force-pushed the chore/393/split-controllers branch from d4b9a68 to 77fce00 Compare March 17, 2023 10:57
@odubajDT odubajDT marked this pull request as ready for review March 17, 2023 12:41
@AlexsJones
Copy link
Member

Can you tell us why this is a good idea perhaps? I've not touched this code in a few months and the delta seems large.

@odubajDT odubajDT changed the title chore: move controllers to separate packages chore: refactoring + improving test coverage Mar 20, 2023
@odubajDT
Copy link
Contributor Author

Can you tell us why this is a good idea perhaps? I've not touched this code in a few months and the delta seems large.

Hello,
n general, packages should be kept as small as possible to make them easy to understand. Therefore we decided to split the controllers package and put each controller to a separate one.

Also this PR converting tests from ginkgo test framework to standard unit-test approach, so it's less flaky, as we experienced some problems with this approach in the past. Additionally more unit tests were added for api/controllers to increase the code coverage.

@odubajDT
Copy link
Contributor Author

This PR is going to be split into 2 due to the size

@odubajDT odubajDT force-pushed the chore/393/split-controllers branch 2 times, most recently from 1d775c4 to 8a1e2db Compare March 21, 2023 06:41
@odubajDT odubajDT changed the title chore: refactoring + improving test coverage chore: split controllers to separate packages + cover them with unit tests Mar 21, 2023
@odubajDT
Copy link
Contributor Author

odubajDT commented Mar 21, 2023

Split of the PR completed, the follow-up PR with additional refactoring and unit tests will follow up

  1. Follow up PR: chore: refactor admission webhook tests #409

Another PR regarding adding unit tests for api packages will come after this will get merged

Makefile Show resolved Hide resolved
Copy link
Contributor

@skyerus skyerus left a comment

Choose a reason for hiding this comment

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

I like the splitting of the controllers package, nice

@odubajDT
Copy link
Contributor Author

I like the splitting of the controllers package, nice

Thank you!

@toddbaert
Copy link
Member

I'll give this a thorough review tomorrow!

@odubajDT
Copy link
Contributor Author

I'll give this a thorough review tomorrow!

Thank you!

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Approved with some minor comments, questions.

@odubajDT odubajDT requested a review from a team as a code owner March 24, 2023 18:01
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT force-pushed the chore/393/split-controllers branch from 50f1cf3 to 2ea30e6 Compare March 24, 2023 18:22
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT force-pushed the chore/393/split-controllers branch from 508ad66 to 155c5bb Compare March 24, 2023 18:48
@toddbaert toddbaert merged commit 6ed4cef into open-feature:main Mar 27, 2023
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.

Move controllers into their own packages
6 participants