I had completely forgotten I wrote this test (#956). I'm not really sure how well the test is designed, so I'm ok with skipping it. What's weird is Pipelines seems to be timing out at 15 seconds but the test shouldn't take that long to fail.

Please review the solution in #1161. The None vs. 0 thing means that in some instances we are never polling.

@cclauss Might you quickly elaborate how that change affects this test? max(5/0.1,1) should evaulate to my understanding in 50, exactly like 5/0.1 does, so although your change makes the function "call_with_timeout" more robust for timeout < 0.1, I do not see how it could lead to a change in the given test, which uses timeout=5.

@cclauss Also, it is possible for Popen.poll() to return None, so from observing None we cannot conclude that no poll has taken place. Here is the documentation for Popen.poll():
https://docs.python.org/3/library/subprocess.html#subprocess.Popen.poll

Sure. After you click thru the 7 links that Azure Pipelines makes you go thru to see why your test failed, you get:

test_pg_exit
Failed 1h ago on Mac-1480.local
Duration0:00:15.360
Failing build20200409.8
Ownernot available

assert None == 0   +None   -0
def test_pg_exit():
        # test the pg.exit() function
        code = textwrap.dedent("""
            import pyqtgraph as pg
            app = pg.mkQApp()
            pg.plot()
            pg.exit()
        """)
        rc = call_with_timeout([sys.executable, '-c', code], timeout=10, shell=False)
>       assert rc == 0
E       assert None == 0
E         +None
E         -0

../pyqtgraph/tests/test_exit_crash.py:80: AssertionError

This clearly indicates that the line rc = p.poll() was never called because that function always returns an int and never returns None. The proposed fix only assures that regardless of how the math works out, this polling loop is run at least once.

@cclauss Please check the following documentation of poll, linked in the post above:

Check if child process has terminated. Set and return returncode attribute. Otherwise, returns None.

You are correct. I was wrong. However this fix still works because it forces the loop to run at least once.

@cclauss One additional note: I had a Pull Request up just some hours ago ( #1159 ) that showed the exact same error, and just re-running the tests, literally without any changes, made all tests pass. This is why I am a bit careful here.

Given that this only occurs on the macOS/python3.6/qt5.9 pipeline, and nowhere else, there is no doubt there is some CI environment issues at play (just like previous versions of macOS really did not like some of the openGL tests).

I'm good with modifying this test if it makes it a bit more robust, even if it is to address a CI environment issue.

@2xB Look at the diff from this commit: 62a1085

I think this is what @cclaus has been suggested we implement.

That doesn't resolve the issue unfortunately - #1164 showed that the loop definitely runs, including on the MacOS-Py3.6-PyQt5.9 setup. This is an intermittent failure on a specific setup, so something tricky is going on.

I mentioned it above, but something that sticks out to me is this line: https://dev.azure.com/pyqtgraph/pyqtgraph/_build/results?buildId=684&view=logs&j=90cc5e1f-49e9-5cbd-c0c1-00a1410b1772&t=47d78265-9244-5a5c-bb6c-e6ac800228fd&l=236

The Timeout (0:00:15)! message is, I think, from Pipelines (we have it configured for 15 second timeout). Understanding how/why that occurs seems important to figuring out what's going on.

I think my vote at this point is to create an issue to keep track of this, skip the test, and move on for now.

@j9ac9k I would be happy about the linked commit by @cclauss to be merged! It makes call_with_timeout more robust when used with different timeouts. My only point was that this is not the end of our journey regarding this issue.

@ixjlyons I've never used pg.exit() before, in its documentation its use is not really advertised and not a single example uses it. So is there even a use for it? Otherwise I would also vote for the suggestion to remove this test from the pipeline completely. Maybe in that case, we could even discuss about making pg.exit just an alias for sys.exit?

Can't believe it took me so long before attempting to replicate locally, which I am able to do.

That said we're super close to killing off pyqt5.9 support i'm debating if we should just skip this test.

So this is not a test environment-specific thing after all? Interesting! Well, for me the question remains what the current use case of pg.exit() over sys.exit() would be anyways and why we don't just make the one an alias of the other.

....so it fails on a reproducible manner, running pytest . on the develop branch right now will fail on m local machine (with PyQt 5.9). If I run pytest . -k "test_pg_exit", that test passes. I'm willing to write this off as a PyQt 5.9 specific issue, and skip the test on that version.

I have no strong opinions on just making pg.exit() an alias to sys.exit()

@2xB @ixjlyons how do you all feel about closing this issue out with the merging of #1215 ?

Okay! The one missing part for me is why pg.exit exists in the first place, and why we have it defined in a custom way, but that might become a new "Issue" at one point and is probably out of scope for this discussion.