Code

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#8551 closed (fixed)

Add REMOTE_ADDR: '127.0.0.1' to TestClient environment by default

Reported by: schmichael Owned by: ericholscher
Component: Testing framework Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The fake environment django.test.client.Client sets up is missing REMOTE_ADDR. It could be very safely and accurately defaulted to localhost (127.0.0.1).

Luckily this issue is very easy to work around when instantiating the test Client:

c = Client(REMOTE_ADDR='127.0.0.1')

Attachments (1)

django-remote_addr-test.patch (498 bytes) - added by schmichael 6 years ago.

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by schmichael

comment:1 Changed 6 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

It is a pretty simple fix and I can't see any potential bad side-effects. In fact, since RFC3875 says that "The REMOTE_ADDR variable MUST be set to the network address of the client sending the request to the server." it seems fair enough to set it in the test client for a reliable test environment.

comment:2 Changed 5 years ago by ericholscher

Just ran into this as well. I fixed it by doing the suggested c = Client(REMOTE_ADDR='127.0.0.1') and it works fine. Not a huge deal, but I agree that I don't think it'll hurt anything, and seems like a sane default.

comment:3 Changed 5 years ago by ericholscher

  • Owner changed from nobody to ericholscher

comment:4 Changed 5 years ago by Rob Hudson <treborhudson@…>

I just helped someone on #django IRC who was getting hung up on this as well. Seems like a quick fix that would save testers some headache.

comment:5 Changed 5 years ago by russellm

I agree that we should do something here. The reason I haven't checked anything in is that my initial thoughts snagged on an issue, and I haven't had a chance to come back to it confirm whether its actually a problem, or if I'm overthinking things.

The point I got snagged on is this: 127.0.0.1 isn't a particularly "remote" REMOTE_ADDR. If you're testing for behavior that will be observed remotely, there could be edge cases that won't be exercised if your test case is inherently local. However, a completely fake IP (e.g., 12.34.56.78) isn't really a good idea either, since it isn't connected to anything in the real world.

However, I am fully willing to admit that this may be a delusion of my deranged mind. In this case, I just need someone to sit me down with a warm cup of cocoa, give me my blanky and say "there there" while rubbing my forehead and telling me that the bad man will go away.

comment:6 Changed 5 years ago by mtredinnick

I think you need to take your blanky and go dip it in your cocoa for a bit. 127.0.0.1 should behave the same as any other IP address, at least for our purposes. It's "just" an IP address: it uses the same network stack and everything (except its routing goes to the local machine). It also happens to be the correct value.

Using a valid IP address makes sense and the 127.0.0.1 address is a reasonable guess. Determining if the machine has a non-local IP address and using that, if so, would also be valid, but a lot of extra work. Only small problem is that 127.0.0.1 is not guaranteed to be the loopback address (I've seen at least one machine in production -- at an ISP -- that used another number for reasons I didn't quite understand). IPv6-only networks also use a different string form there, too, obviously (once again, showing that using phrases like "safely and accurately" and "easily" in a bug description are warning signs). However, determining the loopback address dynamically is fairly tricky (relies on networking being set up correctly in a number of easy-to-get-wrong ways on the machine in question), so I'd vote for punting on being right 100% of the time for now and just go for 98% of the time plus an "XXX" or "FIXME" note about the other 2%.

comment:7 Changed 5 years ago by russellm

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

(In [9847]) Fixed #8551 -- Added REMOTE_ADDR to the default Test Client environment. Thanks to schmichael for the report and patch.

comment:8 Changed 5 years ago by russellm

(In [9848]) [1.0.X] Fixed #8551 -- Added REMOTE_ADDR to the default Test Client environment. Thanks to schmichael for the report and patch.

Merge of r9847 from trunk.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.