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

Regression with parallel tests on 2.7.3 and 2.8 #1083

Closed
xavfernandez opened this Issue Sep 29, 2015 · 22 comments

Comments

Projects
None yet
3 participants
@xavfernandez
Copy link

xavfernandez commented Sep 29, 2015

Hello,
Some tests in pip test suite started failing with latest pytest releases:

@RonnyPfannschmidt RonnyPfannschmidt added this to the 2.8.2 milestone Sep 29, 2015

@RonnyPfannschmidt RonnyPfannschmidt self-assigned this Sep 29, 2015

xavfernandez added a commit to pypa/pip that referenced this issue Sep 29, 2015

Merge pull request #3150 from xavfernandez/pin_pytest_272
Pin pytest==2.7.2 to avoid failing tests

Cf pytest-dev/pytest#1083

@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8.2, 2.8.3 Oct 10, 2015

@xavfernandez xavfernandez referenced this issue Oct 20, 2015

Closed

more logs #3124

@nicoddemus nicoddemus modified the milestones: 2.8.3, 2.8.4 Nov 23, 2015

@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8.4, 2.8.5 Dec 6, 2015

@RonnyPfannschmidt RonnyPfannschmidt modified the milestones: 2.8.5, 2.8.6 Dec 14, 2015

@xavfernandez

This comment has been minimized.

Copy link
Author

xavfernandez commented Jan 5, 2016

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jan 5, 2016

hmm, indeed - i suspect this bug has been lingering longer, the structural change just makes it trigger more easily

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jan 5, 2016

due to race conditions it seems, that the lockfile creation and the folder creation can intersect badly now, since it got a lot faster - i propose switching to a datetime+pid based folder name in order to reduce the likelihood of collisions

@nicoddemus opinions?

i'd suggest a folder name like 2016-01-04+15:46:33+1337 concatenating date, time and pid

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jan 5, 2016

i propose switching to a datetime+pid based folder name in order to reduce the likelihood of collisions

Which folder exactly do you mean @RonnyPfannschmidt? For example, here's what Travis is creating:

 /tmp/pytest-of-travis/pytest-1/popen-gw0/<test-name>
@xavfernandez

This comment has been minimized.

Copy link
Author

xavfernandez commented Jan 19, 2016

Any new idea ? :)

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jan 19, 2016

@RonnyPfannschmidt did you notice my question above? 😁

@xavfernandez

This comment has been minimized.

Copy link
Author

xavfernandez commented Mar 4, 2016

Any advancement ?
I'd like pip to keep on track with pytest 😿

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Mar 4, 2016

i can put a focus on it next week, this has been delayed due to unrelated personal life events

@xavfernandez

This comment has been minimized.

Copy link
Author

xavfernandez commented Apr 7, 2016

Any news ? 😿

@xavfernandez

This comment has been minimized.

Copy link
Author

xavfernandez commented May 19, 2016

Gentle ping, I'd rather not have pip indefinitely stranded on pytest==2.7.2 :-/

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented May 19, 2016

atm i cant promise anything wrt getting at it soonish

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented May 19, 2016

@RonnyPfannschmidt could please you explain in more detail why do you think there is a race condition with the lock creation? I ask because looking at make_numbered_dir, the directory creation should avoid any race condition:

            try:
                udir = rootdir.mkdir(prefix + str(maxnum+1))
            except py.error.EEXIST:
                # race condition: another thread/process created the dir
                # in the meantime.  Try counting again
                if lastmax == maxnum:
                    raise
                lastmax = maxnum
                continue

IIUC, the lock file only exists so a directory in use won't be deleted by a new py.test session, but on Travis we only ever have one py.test session active at once (we create a temp directory for each session, with each slave reusing it).

Also, the original change only changed the tmp root directory and I suspect Travis's VMs contains a very small number of files in /tmp, so I don't see how that could affect the outcome of those tests.

Any hints on what's on your mind about the problem might be helpful. 😁

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 1, 2016

@RonnyPfannschmidt any further thoughts?

Either way, we really should tackle and solve this once and for all in the upcoming pytest sprint, in a couple of weeks.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jul 19, 2016

after some more investigation it seems like the tmpdir initialization/re-initialization at slave startup happens multiple times, once with the xdist configuration, once without

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jul 19, 2016

this is related to #1618

nicoddemus added a commit to nicoddemus/pip that referenced this issue Jul 22, 2016

nicoddemus added a commit to nicoddemus/pip that referenced this issue Jul 22, 2016

Change "tmpdir" fixture to work with latest pytest
Re-using the built-in tmpdir fixture fixes pytest-dev/pytest#1083

Also with latest pytest there's no need to use --assert=plain on py35 anymore

Fixes pypa#3699
Fixes pytest-dev/pytest#1083

nicoddemus added a commit to nicoddemus/pip that referenced this issue Jul 22, 2016

Change "tmpdir" fixture to work with latest pytest
Re-using the built-in tmpdir fixture fixes pytest-dev/pytest#1083

Also with latest pytest there's no need to use --assert=plain on py35 anymore

Fixes pypa#3699
Fixes pytest-dev/pytest#1083

@nicoddemus nicoddemus removed this from the 2.8.6 milestone Jul 22, 2016

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 22, 2016

Opened pypa/pip#3863 which fixes this problem, although I couldn't identify why the original code did not work in the first place.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jul 22, 2016

my current impression is that the initialization of the tmpdir handler on a remote pytest is incorrect and happens twice due to the option application happening after the pytest initialization instead of before

@xavfernandez

This comment has been minimized.

Copy link
Author

xavfernandez commented Jul 22, 2016

Well, it's a good news that pip can now use the latest pytest.
But I'm uncomfortable with the fact we don't now why :-/

I guess you've already checked similar logs (PYTEST_DEBUG = 1) but I've recreated a branch to have more infos on the failure: https://travis-ci.org/pypa/pip/jobs/146602684

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 23, 2016

@xavfernandez agreed, @RonnyPfannschmidt has hunch that the problem might be regarding how options (specially basetemp) is forwarded and handled at the slaves, see #1743.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jul 23, 2016

@xavfernandez did some tests showing that my hunch was wrong

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jul 23, 2016

@RonnyPfannschmidt I was thinking, while #1743 makes sense, that wouldn't explain why their old tmpdir implementation did not work, after all it was correctly using config._tmpdir_factory (see my PR), which is also used by pytest's own tmpdir.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Jul 23, 2016

@nicoddemus yes, ever since @xavfernandez demonstrated that the issue is in fact not related to that, i do wonder what is the actual cause

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.