Opened 15 years ago

Closed 15 years ago

#1963 defect closed fixed (fixed)

getPassword unit test incorrectly uses popen3()

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

This needs to be spawnProcess instead.

This is killing the 2.5 buildslave.

Change History (8)

comment:1 Changed 15 years ago by Jean-Paul Calderone

Keywords: review added
Priority: highhighest

Waiting review in getpassword-1963

comment:2 Changed 15 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to Glyph

comment:3 Changed 15 years ago by Moshe Zadka

I'm beginning to review it

comment:4 Changed 15 years ago by Moshe Zadka

Owner: changed from Glyph to Jean-Paul Calderone

The test is certainly cleaner with spawnProcess. It is kinda sad that it is unix-only, it could probably be portable. I think it should go in.

comment:5 Changed 15 years ago by Jonathan Lange

Owner: changed from Jean-Paul Calderone to Jonathan Lange
Status: newassigned

comment:6 Changed 15 years ago by Jonathan Lange

Keywords: review removed
Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: assignednew

This is 100 times clearer than the original test. Assuming the tests pass, my only thoughts are:

  • Maybe explain why 'unix only'
  • Maybe put a comment somewhere that explains the script prints the result of getPassword() to stdout.

Make those changes if you'd like, then feel free to merge w/out bouncing back for review. Otherwise, merge away.

comment:7 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [17765]) Merge getpassword-1963

Author: exarkun Reviewer: jml Fixes #1963

Replace the usage of popen3 in the test for getPassword with spawnProcess.

comment:8 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.