Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests into source tarball #6

Merged
merged 1 commit into from Oct 8, 2013
Merged

Conversation

tomspur
Copy link
Contributor

@tomspur tomspur commented Oct 8, 2013

Please also add the tests into the source tarball, so it can be also tested downstream, when building the package.

takluyver added a commit that referenced this pull request Oct 8, 2013
Add tests into source tarball
@takluyver takluyver merged commit 1f26e5f into pexpect:master Oct 8, 2013
@takluyver
Copy link
Member

Thanks, makes sense.

@tomspur tomspur deleted the tests-tarball branch October 15, 2013 08:11
@takluyver
Copy link
Member

@tomspur we're up to rc2 now - can you run the tests on your platform, if you haven't already?

@takluyver
Copy link
Member

https://github.com/pexpect/pexpect/releases/download/3.0rc2/pexpect-3.0rc2.tar.gz is the download (the tarball made with setup.py sdist)

@tomspur
Copy link
Contributor Author

tomspur commented Oct 30, 2013

@takluyver It works fine on my machine. On rawhide, there is this failure though, seemingly on all platforms:

======================================================================
FAIL: test_sighup (test_misc.TestCaseMisc)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/pexpect-3.0rc2/tests/test_misc.py", line 174, in test_sighup
    raise AssertionError('Child should have exited.')
AssertionError: Child should have exited.
----------------------------------------------------------------------
Ran 110 tests in 125.229s
FAILED (failures=1)

See: http://koji.fedoraproject.org/koji/taskinfo?taskID=6115742

How can I help to further track it down?

@takluyver
Copy link
Member

How fast are your build machines? It could just be not waiting long enough, but it's passing on Travis, which is usually pretty slow. If that might be the case, find the lines for _ in range(10): in that test (careful, there are two such lines), and change 10 to 100, to see if that fixes the issue.

Otherwise, could the build machines be intercepting SIGHUP somehow? The failure is a test of sending SIGHUP to a child process and checking that it terminates.

That test is a new one - previous versions of pexpect set the child process to ignore SIGHUP, because it caused problems with some use cases. We've made it optional, but left the default as it was before.

@jquast
Copy link
Member

jquast commented Oct 30, 2013

I looked at the code for a while and did some manual testing of this signal handling and the code all seems appropriate so far. I suspected slowness too, but the build.log indicates a decent speed in the "10000 runs of " tests, anyway its simple enough to rule out by making this loop longer.

My only remaining suspicion mirror's Thomas': we are sending a signal from one PID to another within this test, a build environment may be (understandably) restricting that, such as an selinux policy. There is plenty of documentation about Koji (build env) starting here https://fedorahosted.org/koji/wiki but I haven't found any mention of disallowing signals .. or any strict policies other than the expectation that it is done in a chroot'd filesystem.

@takluyver
Copy link
Member

This thread from a year ago says that SELinux was disabled on Koji, because of problems they had been having. Also, SELinux has one 'signal' permission for "Send a signal other than SIGKILL, SIGSTOP, or SIGCHLD. " If that was forbidden, I'd expect at least a couple of other tests to fail, because they rely on SIGINT and SIGWINCH.

@tomspur
Copy link
Contributor Author

tomspur commented Oct 31, 2013

I set the sleep(0.1) to sleep(10.0), which should be similar to increasing the range from 10 to 100, without success (It just needs longer to fail).

Especially rawhide should be slow, as the kernel most likely has debuginformation turned on. I just tried it on a released version and it's also failing. I guess, I'll see if I can find that one out, or I just disable that single test on our build system...

@tomspur
Copy link
Contributor Author

tomspur commented Oct 31, 2013

What happens, if the tests are not running in a shell? Then the shell is not there at all and SIGHUB doesn't make much sense as the shell wasn't killed as it has never existed. I highly guess that is the reason for this thest to not work as mock is calling rpmbuild without a shell, but when running here locally I directly call rpmbuild in a shell.
If SIGHUB should be working anyway, this might also be a python issue, as Popen(cmd, shell=False, ...) is used.

@takluyver
Copy link
Member

I think you mean a terminal, not a shell? I'm not convinced, though - my understanding is that SIGHUP is sent when the controlling terminal is closed, but it should be perfectly possible to send it manually without a controlling terminal. Apparently many daemons use SIGHUP as a trigger to reload their config files. Also, the point of pexpect is that it creates a terminal to run the subprocess in, so I think it should be able to send SIGHUP.

I'll try to reproduce it, though. I think a cron job should get run without a terminal, right?

@takluyver
Copy link
Member

I've pulled that test out and run it as a cron job, and it's still succeeding. This is on Debian testing.

@jquast
Copy link
Member

jquast commented Oct 31, 2013

same here, using bash on osx.

$ . test.env
$ (sleep 1; ./tools/testall.py </dev/null >/tmp/out.txt 2>&1) & disown

neither stdin, stdout, or stderr are terminals in this case. the process is even re-parented!

It'll be a lot of work to get learn and build a koji environment of my own to understand what happens here, but I think thats the only way forward at this point, other than raising SkipTest because it finds an environment variable or some such that indicates that it is being built with koji/rawhide

@takluyver
Copy link
Member

@tomspur can you get in contact with the people who run Koji, and see if there's a reason that SIGHUP might not be received there?

I'll try making an Ubuntu PPA package to see if the same thing occurs in that build environment.

@takluyver
Copy link
Member

Nope, the test succeeds in the Ubuntu PPA build environment, both with Python 2.7 and 3.3. Unless we can work out why it fails on Koji, I think we may have to just go ahead and release anyway, and you'll need to patch that test out to get the package to build.

@tomspur
Copy link
Contributor Author

tomspur commented Nov 4, 2013

Somehow I was git cloing an old version of koji before. When looking at the actual sources, there is signal.signal(signal.SIGHUP, signal.SIG_IGN) as part of daemonizing. When then calling a subprocess, it is unable to receive a SIGHUP:

$ . test.env
$ ipython
In [1]: import signal, subprocess
In [2]: signal.signal(signal.SIGHUP, signal.SIG_IGN)
In [3]: subprocess.call(["./tools/testall.py"])

I'll contact the koji owners and see if the ignoring is really necessary as according to [1] it shouldn't. (I don't have that much experience with subprocesses/signals, but maybe you could comment on that one?)

[1] http://code.activestate.com/recipes/278731-creating-a-daemon-the-python-way/

@tomspur
Copy link
Contributor Author

tomspur commented Nov 4, 2013

Other than that, feel free to release the new version as is, I'll just ignore the failing test, when building in koji for now.

@takluyver
Copy link
Member

Ah, I see. That must be inherited by subprocesses. So the issue is that our test assumes that there's not already a signal handler set for SIGHUP. I've just committed 1fbfddf, which makes the test restore the default handler before it runs. Can you try that on the build machines? I'll make an RC3 tarball shortly.

@takluyver
Copy link
Member

@tomspur
Copy link
Contributor Author

tomspur commented Nov 5, 2013

Great! All tests pass now on python2 and python3 on x86_64, i686 and arm:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6138629

@jquast
Copy link
Member

jquast commented Nov 5, 2013

awesome! Nice solution, @takluyver !

@takluyver
Copy link
Member

Excellent. 😃 We'll give people a couple more days to spot problems with RC 3, but so long as none come up, expect a final release by the end of the week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants