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

Remove code duplication in tests #140

Closed
wants to merge 4 commits into from
Closed

Remove code duplication in tests #140

wants to merge 4 commits into from

Conversation

DeliaPavel
Copy link
Contributor

@DeliaPavel DeliaPavel commented Feb 28, 2022

Create function test_state in state.rs that holds the duplicated code
from function test_invalid_avail_idx from queue.rs and function
test_queue_guard_object from queue_guard.rs

Fixes #122

Signed-off-by: Delia-Maria PAVEL (101431) delia_maria.pavel@stud.acs.upb.ro

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • Any newly added unsafe code is properly documented.

Create function test_state in state.rs that holds the duplicated code
from function test_invalid_avail_idx from queue.rs and function
test_queue_guard_object from queue_guard.rs

Fixes #122

Signed-off-by: Delia-Maria PAVEL (101431) <delia_maria.pavel@stud.acs.upb.ro>
Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

LGTM, can you also fix the git conflict?

// No descriptor chains are consumed at this point.
assert_eq!(q.next_avail(), 0);
assert_eq!(q.next_used(), 0);
assert_eq!(state.next_avail(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this refactoring is that you are no longer calling these methods from the Queue/QueueGuard implementation, but always from the QueueState one. Because of this, having two separate tests doesn't actually make much sense anymore. I'll think about what we can do regarding this.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't find any solution to this without some serious refactoring. One way of achieving this is to implement the same trait on both QueueGuard and Queue, and the test case receive as a parameter a parametrized Q where Q implements the common trait. This is extending far beyond the scope of the opened issue. The code in Queue and QueueGuard is duplicated as well, but I think that needs to be handled as a separate issue

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find any solution to this without some serious refactoring. One way of achieving this is to implement the same trait on both QueueGuard and Queue, and the test case receive as a parameter a parametrized Q where Q implements the common trait. This is extending far beyond the scope of the opened issue. The code in Queue and QueueGuard is duplicated as well, but I think that needs to be handled as a separate issue

I agree, so should we drop this patch for now?

This pull request was closed.
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.

Unify unit test cases of QueueState and QueueStateSync
3 participants