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

[WIP]Add the missing test for enqueueCStorReplica #587

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@farhaanbukhsh

farhaanbukhsh commented Oct 2, 2018

This commit add the initail work in progress for the tests,
this is done in order to get review.
fixes: openebs/openebs#1919

Signed-off-by: Farhaan Bukhsh farhaan.bukhsh@gmail.com

Add the missing test for enqueueCStorReplica
This commit add the initail work in progress for the tests,
this is done in order to get review.

Signed-off-by: Farhaan Bukhsh <farhaan.bukhsh@gmail.com>
@farhaanbukhsh

This comment has been minimized.

Show comment
Hide comment
@farhaanbukhsh

farhaanbukhsh Oct 2, 2018

Hey it's a work in progress would be nice if I can get a review on this, am I doing something wrong because the queue doesn't seem to have any element.

farhaanbukhsh commented Oct 2, 2018

Hey it's a work in progress would be nice if I can get a review on this, am I doing something wrong because the queue doesn't seem to have any element.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 2, 2018

Codecov Report

Merging #587 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #587   +/-   ##
======================================
  Coverage    31.6%   31.6%           
======================================
  Files         138     138           
  Lines       10943   10943           
======================================
  Hits         3458    3458           
  Misses       7223    7223           
  Partials      262     262

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18f4c0c...451c9e7. Read the comment docs.

codecov bot commented Oct 2, 2018

Codecov Report

Merging #587 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #587   +/-   ##
======================================
  Coverage    31.6%   31.6%           
======================================
  Files         138     138           
  Lines       10943   10943           
======================================
  Hits         3458    3458           
  Misses       7223    7223           
  Partials      262     262

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18f4c0c...451c9e7. Read the comment docs.

@Akash4927 Akash4927 changed the title from Add the missing test for enqueueCStorReplica to [WIP]Add the missing test for enqueueCStorReplica Oct 2, 2018

@Akash4927

This comment has been minimized.

Show comment
Hide comment
@Akash4927

Akash4927 Oct 2, 2018

Member

Hi @farhaanbukhsh, can you please provide the issue link in the description?

Member

Akash4927 commented Oct 2, 2018

Hi @farhaanbukhsh, can you please provide the issue link in the description?

@sonasingh46

This comment has been minimized.

Show comment
Hide comment
@sonasingh46

sonasingh46 Oct 2, 2018

Collaborator

@farhaanbukhsh
Thanks.
Please add issue link to get it reviewed.

Collaborator

sonasingh46 commented Oct 2, 2018

@farhaanbukhsh
Thanks.
Please add issue link to get it reviewed.

@farhaanbukhsh

This comment has been minimized.

Show comment
Hide comment

farhaanbukhsh commented Oct 2, 2018

@Akash4927

This comment has been minimized.

Show comment
Hide comment
@Akash4927

Akash4927 Oct 2, 2018

Member

@farhaanbukhsh if you are done with the changes, please remove WIP from PR heading

Member

Akash4927 commented Oct 2, 2018

@farhaanbukhsh if you are done with the changes, please remove WIP from PR heading

@farhaanbukhsh

This comment has been minimized.

Show comment
Hide comment
@farhaanbukhsh

farhaanbukhsh Oct 2, 2018

Does it look good?

farhaanbukhsh commented Oct 2, 2018

Does it look good?

@farhaanbukhsh

This comment has been minimized.

Show comment
Hide comment
@farhaanbukhsh

farhaanbukhsh Oct 2, 2018

I actually wanted a review of you guys on this

farhaanbukhsh commented Oct 2, 2018

I actually wanted a review of you guys on this

@sonasingh46

This comment has been minimized.

Show comment
Hide comment
@sonasingh46

sonasingh46 Oct 2, 2018

Collaborator

@Akash4927 @sonasingh46 openebs/openebs#1919

here it is

It would be great if you put issue link in PR description in format :
(fixes <issue_link>)
@farhaanbukhsh

Collaborator

sonasingh46 commented Oct 2, 2018

@Akash4927 @sonasingh46 openebs/openebs#1919

here it is

It would be great if you put issue link in PR description in format :
(fixes <issue_link>)
@farhaanbukhsh

},
}
q := common.QueueLoad{}
volumeReplicaController.enqueueCStorReplica(&test, q)

This comment has been minimized.

@sonasingh46

sonasingh46 Oct 2, 2018

Collaborator

Good effort @farhaanbukhsh
Ideally speaking we should not test this function as it in turn uses all the inbuilt library functions.
Additionally, pushing into queue won't reflect size immediately as it is a rate limited and non blocking call.
I cannot think of any cases where we might need to test the function.

@sonasingh46

sonasingh46 Oct 2, 2018

Collaborator

Good effort @farhaanbukhsh
Ideally speaking we should not test this function as it in turn uses all the inbuilt library functions.
Additionally, pushing into queue won't reflect size immediately as it is a rate limited and non blocking call.
I cannot think of any cases where we might need to test the function.

This comment has been minimized.

@farhaanbukhsh

farhaanbukhsh Oct 3, 2018

ahh okay so what should I be testing 😞 ?

@farhaanbukhsh

farhaanbukhsh Oct 3, 2018

ahh okay so what should I be testing 😞 ?

@sonasingh46

This comment has been minimized.

Show comment
Hide comment
@sonasingh46

sonasingh46 Oct 8, 2018

Collaborator

Will need to see the testing scope for the file.
As of now,closing this PR.

Collaborator

sonasingh46 commented Oct 8, 2018

Will need to see the testing scope for the file.
As of now,closing this PR.

@sonasingh46 sonasingh46 closed this Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment