Skip to main content

[pkg-discuss] Re: Review request for 15813784 logging to stderr/stdout broken for svc:/application/pkg/server

  • From: Erik Trauschke < >
  • To:
  • Subject: [pkg-discuss] Re: Review request for 15813784 logging to stderr/stdout broken for svc:/application/pkg/server
  • Date: Thu, 24 Oct 2013 11:00:21 -0700



On 10/24/13 10:35 AM, Xiaobo Shen wrote:
On 10/ 2/13 04:34 PM, Xiaobo Shen wrote:
On 10/ 2/13 04:01 PM, Xiaobo Shen wrote:
Hi guys,

I just applied the suggested fix and wrote a test case for this one.
Let me know if the test case
makes sense.
https://ips.java.net/webrev/xiaoshen/Bug_15813784_pkgsvc/

Xiaobo
I found in the bug descrition, we need to check whether stdout is a
tty, but in the if condition:
if not os.environ.get("PKGDEPOT_CONTROLLER") and \
            not os.isatty(sys.stdin.fileno()):

it actually stdin. let me know which one needs to check. Thanks.

here is a updated webrev check stdout not a tty. I forgot to put
comment in the first.
https://ips.java.net/webrev/xiaoshen/Bug_15813784_pkgsvc2/

Xiaobo
just a kindly reminder. This one has not get review yet.

Sorry about that.

My knowledge on the depot controller is limited but I don't really see in your test case how you determine that the output to stderr actually works. Also, if I run this test in an unpatched gate it will succeed, which it shouldn't.

The fix it self looks ok, only the test case needs to be fixed.

I know this one is kinda tricky but it should be possible to start up the depot with cmdline_run and forward stdout into a file. This should be enough to trigger the behavior we want. You then have to somehow make the depot write to stderr. You can scan through the code of the depot to find out what are situations when errors are printed and chose one which is easy to reproduce. Then you just have to check if something ends up in self.errout.

Make sure you check that the test case you come up with will fail if your fix is not applied (just by hand testing, this does not need to be part of the unit test). Only this way you can verify if your test case is actually valid.

Erik


[pkg-discuss] Review request for 15813784 logging to stderr/stdout broken for svc:/application/pkg/server

Xiaobo Shen 10/02/2013

[pkg-discuss] Re: Review request for 15813784 logging to stderr/stdout broken for svc:/application/pkg/server

Xiaobo Shen 10/02/2013

[pkg-discuss] Re: Review request for 15813784 logging to stderr/stdout broken for svc:/application/pkg/server

Xiaobo Shen 10/24/2013

[pkg-discuss] Re: Review request for 15813784 logging to stderr/stdout broken for svc:/application/pkg/server

Erik Trauschke 10/24/2013
 
 
Close
loading
Please Confirm
Close