Project

General

Profile

Task #3372

UnixStreamChannel: test suite improvement

Added by Junxiao Shi almost 5 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Faces
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
3.00 h

Description

Improve UnixStreamChannel test suite with coverage for these functions and situations:

  • accept multiple incoming connections, and check each face works
  • after a face is closed, it's removed from the channel

Reference: commit:65caf200924b28748037750449e28bcb548dbc9c unix-stream.t.cpp, MultipleAccepts test case.


Related issues

Blocks NFD - Task #3514: Merge Channel test cases as templateClosedDavide Pesavento

Actions
#1

Updated by Spencer Lee over 4 years ago

  • Status changed from New to In Progress
#2

Updated by Davide Pesavento over 4 years ago

This task is in progress but has no assignee. Who's working on it?

#3

Updated by Spencer Lee over 4 years ago

  • Assignee set to Spencer Lee
#4

Updated by Spencer Lee over 4 years ago

  • Status changed from In Progress to Code review
#5

Updated by Junxiao Shi over 4 years ago

  • Blocks Task #3514: Merge Channel test cases as template added
#6

Updated by Spencer Lee over 4 years ago

This needs a major overhaul... try to model the test cases after TestTcpChannel.

I think UnixStreamChannel can have a onConnect and connect for clients, and will include different face manipulation - if interests and data sending need to be tested here, but other than that, isn't UnixStreamChannel different enough from tcp and udp channels that UnixStreamChannel would not be able to share a template?

From what I understand UnixStreamChannel does not use ports, does not use ipv4 or ipv6 AddressTypes, and uses a protocol::socket client instead of a type of channel?

#7

Updated by Davide Pesavento over 4 years ago

Don't worry about sharing the test implementation via templates. That will be dealt with in #3514. Just try to give the test suite the same structure as TestTcpChannel, as much as you can, but they don't necessarily have to be identical.

#8

Updated by Spencer Lee over 4 years ago

Uncertainties in implementation on my end, trying to understand UnixStream more at this point.

  • differences between UnixStreamChannel vs. tcp and udp channel tests - connecting, passing socket for UnixStreamTransport into this->connect method, concept of endpoints
    • are there clientFaces to be monitored as in tcp and udp channel tests? Do they work in the same way, so that face closing can be tested.
#9

Updated by Spencer Lee over 4 years ago

What tests are still not met for UnixStream?

Are these relevant to UnixStreamChannel?

  • connecting to the same endpoint twice doesn't create two different faces, but the first face is reused for the second connect()

  • connecting to an endpoint that isn't listening (or doesn't exist) times out and invokes the FaceCreationFailedCallback

#10

Updated by Davide Pesavento over 4 years ago

Spencer Lee wrote:

Are these relevant to UnixStreamChannel?

  • connecting to the same endpoint twice doesn't create two different faces, but the first face is reused for the second connect()

  • connecting to an endpoint that isn't listening (or doesn't exist) times out and invokes the FaceCreationFailedCallback

UnixStreamChannel doesn't have a connect method, so neither of them makes sense.

#11

Updated by Spencer Lee over 4 years ago

when rebasing a branch, usually a merge is done, or in some work flows i've seen push --force used to reduce the number of merges.

For NDN should a merge be done to complete the rebase?

#12

Updated by Davide Pesavento over 4 years ago

Spencer Lee wrote:

when rebasing a branch, usually a merge is done, or in some work flows i've seen push --force used to reduce the number of merges.

For NDN should a merge be done to complete the rebase?

I don't understand the question. Assuming that with "merge" you mean a "merge commit", a rebase is usually done to avoid it down the line, when your branch/commit is approved and submitted to the master branch. (This is called a fast-forward merge btw). So I really don't understand the question. Tell us what you're trying to do.

As far as push --force is concerned, you don't need to add --force when pushing a new patch set to gerrit. As long as the Change-Id stays the same, gerrit will handle the new push automatically for you and do the right thing.

#13

Updated by Spencer Lee over 4 years ago

You had a comment on the commit Message for patch #5 that said rebase - I was wondering who that was for

http://gerrit.named-data.net/#/c/2738/5//COMMIT_MSG

I'm working on a branch that isn't up-to-date, I was wondering if that was a problem right now.

I think an option is fetch and pull, which will add a merge to the git log - I think that is undesired seeing that the log has no merges.

I can also update my branch then rebase, but then there will be commits that are different and push --force or a merge is necessary I believe - which is the situation I was thinking about in the earlier post.

Or I do nothing and these issues are resolved when the task is complete and ready to be merged.

#14

Updated by Davide Pesavento over 4 years ago

Spencer Lee wrote:

I think an option is fetch and pull, which will add a merge to the git log - I think that is undesired seeing that the log has no merges.

No, don't do this.

I can also update my branch then rebase, but then there will be commits that are different and push --force or a merge is necessary I believe - which is the situation I was thinking about in the earlier post.

Yes, perform a rebase, but as I said earlier, --force is not needed when pushing because gerrit will handle it correctly regardless (it's as if you were pushing to a new branch each time you push a new patchset to gerrit).

Or I do nothing and these issues are resolved when the task is complete and ready to be merged.

This is debatable, but sooner or later the rebase will have to be done, so why not doing it whenever you're pushing an updated patchset? Rebasing is a trivial operation 99% of the time, and takes few seconds.

#15

Updated by Spencer Lee over 4 years ago

Ok I see, thank you.

I didn't know gerrit handled it automatically - I'll rebase from now on.

#16

Updated by Davide Pesavento over 4 years ago

  • Status changed from Code review to Closed
  • % Done changed from 0 to 100

Also available in: Atom PDF