Opened 9 years ago

Closed 9 years ago

#346 closed defect (fixed)

A notify_out test fails without network connectivity

Reported by: jinmei Owned by: zzchen_pku
Priority: low Milestone: A-Team-Sprint-20110223
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.25 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

test_send_notify_message_udp requires sendto() for a packet destined to 1.1.1.1 succeed, so it fails when the testing node is disconnected.

This is bad itself, of course, but I think we should revisit the test design in the first place:

  • IMO it should be generally avoided to rely on system level functionality (such as sendto()) for unit tests unless we want to test that exact functionality.
  • if there's a reason we cannot avoid that, we should at least avoid relying on sending packets outside the testing node. it's very fragile, and could be harmful for others due to the unnecessary traffic.

Ideally we should encapsulate the socket behavior using some mock class. If it's impossible (although I don't think so) we should at least use a loopack address.

Another point that could be improved for notify_out_test: we shouldn't use exisiting domain names for testing such as ".cn" or ".com". We should use "example.com", ".example", etc.

I'm giving it to likun.

Subtickets

Change History (9)

comment:1 Changed 9 years ago by stephen

  • Milestone changed from y2 12 month milestone to A-Team-Sprint-20110223
  • Priority changed from major to minor

comment:2 Changed 9 years ago by zzchen_pku

  • Owner changed from zhanglikun to zzchen_pku
  • Status changed from new to assigned

Since it's an A-team task now, I'm stealing it from Likun.

comment:3 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to UnAssigned
  • Status changed from assigned to reviewing

It's ready for review.
I added a mock socket class for testing and substituted "cn", "com","org" with "example.net", "example.com", "example.org".

comment:4 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:5 follow-up: Changed 9 years ago by vorner

  • Owner changed from vorner to zzchen_pku

Hello

Most of the change is search and replace of the cn. to example.net., etc, which is OK.

However, I find the mock socket class strange. The socket works like a pipe to itself ‒ whatever you send, you can then read from the same socket. I accept that it works currently, but if any other test wanted to use such mock socket, it could cause quite a surprise.

May I suggest to change the mock class the recvfrom and sendto work on one of the sockets from socketpair and the other one is provided from some function to the testing end? (therefore, one would be the „local“ part of the socket for the application and the one that the test could request from it would be the fake „remote“ end). It would allow reading and the data that actually were sent to the socket as a bonus.

comment:6 in reply to: ↑ 5 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

May I suggest to change the mock class the recvfrom and sendto work on one of the sockets from socketpair and the other one is provided from some function to the testing end? (therefore, one would be the „local“ part of the socket for the application and the one that the test could request from it would be the fake „remote“ end). It would allow reading and the data that actually were sent to the socket as a bonus.

Make sense, I have updated MockSocket?.

comment:7 Changed 9 years ago by vorner

  • Owner changed from vorner to zzchen_pku

Thanks, this looks OK.

So, feel free to merge.

comment:8 Changed 9 years ago by zzchen_pku

Proposed ChangeLog? entry:

173.  [bug]       jerry
  python/isc/notify: A notify_out test fails without network connectivity, encapsulate the
  socket behavior using a mock socket class to fix it.
  (Trac #346, git TBD)

comment:9 Changed 9 years ago by zzchen_pku

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged, thanks.

Note: See TracTickets for help on using tickets.