-
Notifications
You must be signed in to change notification settings - Fork 428
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
REST and CLI API for domains #3058
Conversation
Codecov Report
@@ Coverage Diff @@
## multi-tenancy-phase-1 #3058 +/- ##
=========================================================
+ Coverage 78.09% 78.70% +0.60%
=========================================================
Files 374 387 +13
Lines 31219 31576 +357
=========================================================
+ Hits 24382 24851 +469
+ Misses 6837 6725 -112
Continue to review full report at Codecov.
|
Breaks dialyzer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that mongoose_domain_h
and service_admin_extra_domain
are insanely similar to each other, I'm sad about the repeated code 😿 But I don't care about that, I'm mostly concerned, why basically not a single error-path has test coverage? Are we only testing happy cases? Can that be improved?
…nnot_insert_domain_twice_with_the_another_host_type
…t_domain_with_unknown_host_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test coverage is much better now, but codecov shows some missing coverage for deleting domains errors, can we throw a bunch of tests there as well?
Co-authored-by: Nelson Vides <nelson.vides@erlang-solutions.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I added a few minor remarks. There are two more observations:
- Adding tests for unhappy paths in the REST API would be good. This can be done later.
- Newly added modules are missing specs, adding them in subsequent PR's would be nice as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! A lot of tests! Only one comment from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fine. Tests could have been organised to run in parallel when possible and then cut suite time, but anyway, we can leave that for the future.
This PR addresses "REST and CLI API for domains".
Proposed changes include: