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

Move test environment creation into utils. #297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

artek-koltun
Copy link
Contributor

Frontend, middleend and backend used the same procedures to prepare test environment. In this patch, this test environment creation is moved into server/utils.go file which can be used by other packages.

Frontend, middleend and backend used the same procedures
to prepare test environment. In this patch, this test
environment creation is moved into utils.go file which can
be used by other packages.

Signed-off-by: Artsiom Koltun <[email protected]>
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #297 (a1c8fb7) into main (d36704d) will decrease coverage by 2.08%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
- Coverage   71.53%   69.46%   -2.08%     
==========================================
  Files          14       14              
  Lines        1711     1762      +51     
==========================================
  Hits         1224     1224              
- Misses        441      492      +51     
  Partials       46       46              
Impacted Files Coverage Δ
pkg/server/utils.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@artek-koltun artek-koltun marked this pull request as ready for review March 24, 2023 14:37
@artek-koltun artek-koltun requested a review from a team as a code owner March 24, 2023 14:37
Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

why do we still need createTestEnvironment() ? can we just call server.CreateTestEnvironment instead directly ? if we already unifying the calls ?

@artek-koltun
Copy link
Contributor Author

why do we still need createTestEnvironment() ? can we just call server.CreateTestEnvironment instead directly ? if we already unifying the calls ?

We need to modify some internal structures for tests like set a subsystem/controller/volume etc. That's why we need to expose pkg Server at the moment. We can try to cast the server to the original one, but we need to modify the behavior in many tests and I am trying to keep PRs/commits small. If you are not insisting, I will try to come up with a proposal in a next PR

@glimchb
Copy link
Member

glimchb commented Mar 24, 2023

I will try to come up with a proposal in a next PR

I'm just asking, trying to understand. not saying good/bad, just clarification

@artek-koltun
Copy link
Contributor Author

artek-koltun commented Mar 24, 2023

I will try to come up with a proposal in a next PR

I'm just asking, trying to understand. not saying good/bad, just clarification

In addition, we cannot create pkg Servers from utils, due to circular dependency, For example, frontend test requires utils to create a test environment, and utils requires frontend to create Server...

@glimchb
Copy link
Member

glimchb commented Mar 24, 2023

In addition, we cannot create pkg Servers from utils, due to circular dependency, For example, frontend test requires utils to create a test environment, and utils requires frontend to create Server...

good point, keep forgetting it

@intelfisz
Copy link

In addition, we cannot create pkg Servers from utils, due to circular dependency, For example, frontend test requires utils to create a test environment, and utils requires frontend to create Server...

good point, keep forgetting it

@glimchb - it seems all points are addressed, please merge

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.

3 participants