Re: [Lldb-commits] [lldb] r324743 - [Testsuite] Remove leak tests, it's not useful anymore.

2018-02-12 Thread Davide Italiano via lldb-commits 
Apologies, I clearly misunderstood. I'll add this again.

On Mon, Feb 12, 2018 at 2:52 AM, Pavel Labath via lldb-commits <
lldb-commits@lists.llvm.org> wrote:

> I think you misunderstood the purpose the purpose of this test. The
> test was there to make sure that *lldb* does not leak file descriptors
> into the inferior (when I wrote it a couple of years ago, we were
> leaking about half a dozen of them). The python version check was
> added there just because we found there are some leaks coming from
> python itself that we could not control (so we skipped the test with
> those versions).
>
> With this in mind, I think the test is still very much useful. I think
> a better cleanup would be to rewrite this test to not depend on python
> so much, e.g. by driving the test from a command line lldb client a'la
> the tests in `lldb/lit/Expr` (unfortunately I can't think of a way to
> check that an FD is not leaked in a different way than with a
> full-scale integration test).
>
> On 9 February 2018 at 22:48, Vedant Kumar via lldb-commits
>  wrote:
> > Nice!
> >
> > vedant
> >
> >> On Feb 9, 2018, at 8:06 AM, Davide Italiano via lldb-commits <
> lldb-commits@lists.llvm.org> wrote:
> >>
> >> Author: davide
> >> Date: Fri Feb  9 08:06:39 2018
> >> New Revision: 324743
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=324743=rev
> >> Log:
> >> [Testsuite] Remove leak tests, it's not useful anymore.
> >>
> >> This only worked on MacOS, which now ships a newer version of
> >> python without this bug. As such, we don't leak the fd, and
> >> this test is not needed anymore (as it also hardcoded the python
> >> version in the check).
> >>
> >> Removed:
> >>lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-
> leak/Makefile
> >>lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-
> leak/TestFdLeak.py
> >>lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-
> leak/main.c
> >>
> >> Removed: lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/avoids-fd-leak/Makefile
> >> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/
> Python/lldbsuite/test/functionalities/avoids-fd-
> leak/Makefile?rev=324742=auto
> >> 
> ==
> >> --- lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/avoids-fd-leak/Makefile (original)
> >> +++ lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/avoids-fd-leak/Makefile (removed)
> >> @@ -1,5 +0,0 @@
> >> -LEVEL = ../../make
> >> -
> >> -C_SOURCES := main.c
> >> -
> >> -include $(LEVEL)/Makefile.rules
> >>
> >> Removed: lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/avoids-fd-leak/TestFdLeak.py
> >> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/
> Python/lldbsuite/test/functionalities/avoids-fd-
> leak/TestFdLeak.py?rev=324742=auto
> >> 
> ==
> >> --- lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/avoids-fd-leak/TestFdLeak.py (original)
> >> +++ lldb/trunk/packages/Python/lldbsuite/test/
> functionalities/avoids-fd-leak/TestFdLeak.py (removed)
> >> @@ -1,108 +0,0 @@
> >> -"""
> >> -Test whether a process started by lldb has no extra file descriptors
> open.
> >> -"""
> >> -
> >> -from __future__ import print_function
> >> -
> >> -
> >> -import os
> >> -import lldb
> >> -from lldbsuite.test import lldbutil
> >> -from lldbsuite.test.lldbtest import *
> >> -from lldbsuite.test.decorators import *
> >> -
> >> -
> >> -def python_leaky_fd_version(test):
> >> -import sys
> >> -# Python random module leaks file descriptors on some versions.
> >> -if sys.version_info >= (2, 7, 8) and sys.version_info < (2, 7, 10):
> >> -return "Python random module leaks file descriptors in this
> python version"
> >> -return None
> >> -
> >> -
> >> -class AvoidsFdLeakTestCase(TestBase):
> >> -
> >> -NO_DEBUG_INFO_TESTCASE = True
> >> -
> >> -mydir = TestBase.compute_mydir(__file__)
> >> -
> >> -@expectedFailure(python_leaky_fd_version, "bugs.freebsd.org/197376
> ")
> >> -@expectedFailureAll(
> >> -oslist=['freebsd'],
> >> -bugnumber="llvm.org/pr25624 still failing with Python 2.7.10")
> >> -# The check for descriptor leakage needs to be implemented
> differently
> >> -# here.
> >> -@skipIfWindows
> >> -@skipIfTargetAndroid()  # Android have some other file descriptors
> open by the shell
> >> -@skipIfDarwinEmbedded #   # debugserver
> on ios has an extra fd open on launch
> >> -def test_fd_leak_basic(self):
> >> -self.do_test([])
> >> -
> >> -@expectedFailure(python_leaky_fd_version, "bugs.freebsd.org/197376
> ")
> >> -@expectedFailureAll(
> >> -oslist=['freebsd'],
> >> -bugnumber="llvm.org/pr25624 still failing with Python 2.7.10")
> >> -# The check for descriptor leakage needs to be

Re: [Lldb-commits] [lldb] r324743 - [Testsuite] Remove leak tests, it's not useful anymore.

2018-02-12 Thread Pavel Labath via lldb-commits 
I think you misunderstood the purpose the purpose of this test. The
test was there to make sure that *lldb* does not leak file descriptors
into the inferior (when I wrote it a couple of years ago, we were
leaking about half a dozen of them). The python version check was
added there just because we found there are some leaks coming from
python itself that we could not control (so we skipped the test with
those versions).

With this in mind, I think the test is still very much useful. I think
a better cleanup would be to rewrite this test to not depend on python
so much, e.g. by driving the test from a command line lldb client a'la
the tests in `lldb/lit/Expr` (unfortunately I can't think of a way to
check that an FD is not leaked in a different way than with a
full-scale integration test).

On 9 February 2018 at 22:48, Vedant Kumar via lldb-commits
 wrote:
> Nice!
>
> vedant
>
>> On Feb 9, 2018, at 8:06 AM, Davide Italiano via lldb-commits 
>>  wrote:
>>
>> Author: davide
>> Date: Fri Feb  9 08:06:39 2018
>> New Revision: 324743
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=324743=rev
>> Log:
>> [Testsuite] Remove leak tests, it's not useful anymore.
>>
>> This only worked on MacOS, which now ships a newer version of
>> python without this bug. As such, we don't leak the fd, and
>> this test is not needed anymore (as it also hardcoded the python
>> version in the check).
>>
>> Removed:
>>
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
>>
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
>>
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/main.c
>>
>> Removed: 
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
>> URL: 
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile?rev=324742=auto
>> ==
>> --- 
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
>>  (original)
>> +++ 
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
>>  (removed)
>> @@ -1,5 +0,0 @@
>> -LEVEL = ../../make
>> -
>> -C_SOURCES := main.c
>> -
>> -include $(LEVEL)/Makefile.rules
>>
>> Removed: 
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
>> URL: 
>> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py?rev=324742=auto
>> ==
>> --- 
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
>>  (original)
>> +++ 
>> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
>>  (removed)
>> @@ -1,108 +0,0 @@
>> -"""
>> -Test whether a process started by lldb has no extra file descriptors open.
>> -"""
>> -
>> -from __future__ import print_function
>> -
>> -
>> -import os
>> -import lldb
>> -from lldbsuite.test import lldbutil
>> -from lldbsuite.test.lldbtest import *
>> -from lldbsuite.test.decorators import *
>> -
>> -
>> -def python_leaky_fd_version(test):
>> -import sys
>> -# Python random module leaks file descriptors on some versions.
>> -if sys.version_info >= (2, 7, 8) and sys.version_info < (2, 7, 10):
>> -return "Python random module leaks file descriptors in this python 
>> version"
>> -return None
>> -
>> -
>> -class AvoidsFdLeakTestCase(TestBase):
>> -
>> -NO_DEBUG_INFO_TESTCASE = True
>> -
>> -mydir = TestBase.compute_mydir(__file__)
>> -
>> -@expectedFailure(python_leaky_fd_version, "bugs.freebsd.org/197376")
>> -@expectedFailureAll(
>> -oslist=['freebsd'],
>> -bugnumber="llvm.org/pr25624 still failing with Python 2.7.10")
>> -# The check for descriptor leakage needs to be implemented differently
>> -# here.
>> -@skipIfWindows
>> -@skipIfTargetAndroid()  # Android have some other file descriptors open 
>> by the shell
>> -@skipIfDarwinEmbedded #   # debugserver on ios 
>> has an extra fd open on launch
>> -def test_fd_leak_basic(self):
>> -self.do_test([])
>> -
>> -@expectedFailure(python_leaky_fd_version, "bugs.freebsd.org/197376")
>> -@expectedFailureAll(
>> -oslist=['freebsd'],
>> -bugnumber="llvm.org/pr25624 still failing with Python 2.7.10")
>> -# The check for descriptor leakage needs to be implemented differently
>> -# here.
>> -@skipIfWindows
>> -@skipIfTargetAndroid()  # Android have some other file descriptors open 
>> by the shell
>> -@skipIfDarwinEmbedded #   # debugserver on ios 
>> has an extra fd open on launch
>> -def test_fd_leak_log(self):
>> -self.do_test(["log enable -f '/dev/null' lldb commands"])
>>

Re: [Lldb-commits] [lldb] r324743 - [Testsuite] Remove leak tests, it's not useful anymore.

2018-02-09 Thread Vedant Kumar via lldb-commits 
Nice!

vedant

> On Feb 9, 2018, at 8:06 AM, Davide Italiano via lldb-commits 
>  wrote:
> 
> Author: davide
> Date: Fri Feb  9 08:06:39 2018
> New Revision: 324743
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=324743=rev
> Log:
> [Testsuite] Remove leak tests, it's not useful anymore.
> 
> This only worked on MacOS, which now ships a newer version of
> python without this bug. As such, we don't leak the fd, and
> this test is not needed anymore (as it also hardcoded the python
> version in the check).
> 
> Removed:
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
>
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/main.c
> 
> Removed: 
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile?rev=324742=auto
> ==
> --- 
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
>  (original)
> +++ 
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/Makefile
>  (removed)
> @@ -1,5 +0,0 @@
> -LEVEL = ../../make
> -
> -C_SOURCES := main.c
> -
> -include $(LEVEL)/Makefile.rules
> 
> Removed: 
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py?rev=324742=auto
> ==
> --- 
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
>  (original)
> +++ 
> lldb/trunk/packages/Python/lldbsuite/test/functionalities/avoids-fd-leak/TestFdLeak.py
>  (removed)
> @@ -1,108 +0,0 @@
> -"""
> -Test whether a process started by lldb has no extra file descriptors open.
> -"""
> -
> -from __future__ import print_function
> -
> -
> -import os
> -import lldb
> -from lldbsuite.test import lldbutil
> -from lldbsuite.test.lldbtest import *
> -from lldbsuite.test.decorators import *
> -
> -
> -def python_leaky_fd_version(test):
> -import sys
> -# Python random module leaks file descriptors on some versions.
> -if sys.version_info >= (2, 7, 8) and sys.version_info < (2, 7, 10):
> -return "Python random module leaks file descriptors in this python 
> version"
> -return None
> -
> -
> -class AvoidsFdLeakTestCase(TestBase):
> -
> -NO_DEBUG_INFO_TESTCASE = True
> -
> -mydir = TestBase.compute_mydir(__file__)
> -
> -@expectedFailure(python_leaky_fd_version, "bugs.freebsd.org/197376")
> -@expectedFailureAll(
> -oslist=['freebsd'],
> -bugnumber="llvm.org/pr25624 still failing with Python 2.7.10")
> -# The check for descriptor leakage needs to be implemented differently
> -# here.
> -@skipIfWindows
> -@skipIfTargetAndroid()  # Android have some other file descriptors open 
> by the shell
> -@skipIfDarwinEmbedded #   # debugserver on ios 
> has an extra fd open on launch
> -def test_fd_leak_basic(self):
> -self.do_test([])
> -
> -@expectedFailure(python_leaky_fd_version, "bugs.freebsd.org/197376")
> -@expectedFailureAll(
> -oslist=['freebsd'],
> -bugnumber="llvm.org/pr25624 still failing with Python 2.7.10")
> -# The check for descriptor leakage needs to be implemented differently
> -# here.
> -@skipIfWindows
> -@skipIfTargetAndroid()  # Android have some other file descriptors open 
> by the shell
> -@skipIfDarwinEmbedded #   # debugserver on ios 
> has an extra fd open on launch
> -def test_fd_leak_log(self):
> -self.do_test(["log enable -f '/dev/null' lldb commands"])
> -
> -def do_test(self, commands):
> -self.build()
> -exe = self.getBuildArtifact("a.out")
> -
> -for c in commands:
> -self.runCmd(c)
> -
> -target = self.dbg.CreateTarget(exe)
> -
> -process = target.LaunchSimple(
> -None, None, self.get_process_working_directory())
> -self.assertTrue(process, PROCESS_IS_VALID)
> -
> -self.assertTrue(
> -process.GetState() == lldb.eStateExited,
> -"Process should have exited.")
> -self.assertTrue(
> -process.GetExitStatus() == 0,
> -"Process returned non-zero status. Were incorrect file 
> descriptors passed?")
> -
> -@expectedFailure(python_leaky_fd_version, "bugs.freebsd.org/197376")
> -@expectedFailureAll(
> -oslist=['freebsd'],
> -bugnumber="llvm.org/pr25624 still failing with Python 2.7.10")
> -# The check for descriptor leakage needs to be implemented differently
> -# here.
> -