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

Bump zope.testrunner / fix test isolation problems #155

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
5 participants
@gforcada
Contributor

gforcada commented Jan 3, 2016

Applies back #72 after it was reverted on #154.

@do3cc nailed why a minor version bump of zope.testrunner makes our tests fail: mixing ZopeTestCase and plone.app.testing within the same distribution renders all its tests unstable.

Note that fixing these test isolation problems not only has the benefit of having more robust tests, but at the same time our test infrastructure will be way more easy (and with that making it far easier to reproduce bugs that are only triggered in Jenkins).

Another nice bonus is that unit tests will run first and thus early feedback if you are already expecting failures.

Lastly a slight increase on speed, due to less layer setup and teardown should be observed as well.

The list of packages affected are:

please create separate tickets for each of them and link them back here so we can keep track of it

  • archetypes.multilingual
  • borg.localrole
  • plone.app.blob
  • plone.app.content
  • plone.app.contentlisting
  • plone.app.customerize
  • plone.app.i18n
  • plone.app.layout
  • plone.app.multilingual
  • plone.app.portlets
  • plone.app.testing
  • plone.app.upgrade
  • plone.app.workflow
  • plone.app.z3cform
  • plone.browserlayer
  • plone.openid
  • plone.session
  • plone.stringinterp
  • plone.testing
  • plone.theme
  • Products.Archetypes
  • Products.ATContentTypes
  • Products.CMFEditions
  • Products.CMFPlacefulWorkflow
  • Products.CMFPlone
  • Products.Marshall
@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Jan 4, 2016

Contributor

Ok, so nothing wrong: jenkins does something like:

python bootstrap.py
bin/buildout
bin/alltests --all
bin/alltests-at

On the pull request job it already gives an (unrelated) error on the third command (bin/alltest --all) and for whichever reason the build stops and does not run the last command.

Contributor

gforcada commented Jan 4, 2016

Ok, so nothing wrong: jenkins does something like:

python bootstrap.py
bin/buildout
bin/alltests --all
bin/alltests-at

On the pull request job it already gives an (unrelated) error on the third command (bin/alltest --all) and for whichever reason the build stops and does not run the last command.

@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Jan 4, 2016

Contributor

My finding so far on what's failing: the errors are while running Products.CMFDiffTool and plone.app.testing tests.

It looks like there are layer setup/teardown mistakes in CMFDiffTool:

On zope.testrunner 4.4.4 the AT layer is the first layer to be run, works perfectly and the rest of the layers run fine as well.

On zope.testrunner > 4.4.4 the AT layer is ran after other layers have been executed and it fails with a ValueError: undefined property 'content_meta_type' which looking at plone/Products.CMFPlone#382 makes me think that some layers are leaking some content type data and thus making the AT layer to fail while setting up.

Minimal way to trigger it:

  • get a 4.3 buildout.coredev and switch to this branch
  • run ./bin/test -s Products.CMFDiffTool -t testATCompoundDiff -t TestTextDiff -t TestChangeSet

@tisto @jensens does this look right?

Contributor

gforcada commented Jan 4, 2016

My finding so far on what's failing: the errors are while running Products.CMFDiffTool and plone.app.testing tests.

It looks like there are layer setup/teardown mistakes in CMFDiffTool:

On zope.testrunner 4.4.4 the AT layer is the first layer to be run, works perfectly and the rest of the layers run fine as well.

On zope.testrunner > 4.4.4 the AT layer is ran after other layers have been executed and it fails with a ValueError: undefined property 'content_meta_type' which looking at plone/Products.CMFPlone#382 makes me think that some layers are leaking some content type data and thus making the AT layer to fail while setting up.

Minimal way to trigger it:

  • get a 4.3 buildout.coredev and switch to this branch
  • run ./bin/test -s Products.CMFDiffTool -t testATCompoundDiff -t TestTextDiff -t TestChangeSet

@tisto @jensens does this look right?

@tisto

This comment has been minimized.

Show comment
Hide comment
@tisto

tisto Jan 4, 2016

Member

We see these kind of problems all the time. Most of our old tests just pass because they are ran in a specific order. Changing the order when older packages are involved almost always leads to problems. We have tons of test isolation issues hidden in our setup. So far we were just able to fix the most obvious ones.

The error "ValueError: undefined property 'content_meta_type'" is also well known. See plone/Products.CMFPlone#382 for details.

Member

tisto commented Jan 4, 2016

We see these kind of problems all the time. Most of our old tests just pass because they are ran in a specific order. Changing the order when older packages are involved almost always leads to problems. We have tons of test isolation issues hidden in our setup. So far we were just able to fix the most obvious ones.

The error "ValueError: undefined property 'content_meta_type'" is also well known. See plone/Products.CMFPlone#382 for details.

@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Jan 4, 2016

Contributor

@tisto ok, thanks, so at least I'm right on diagnosing the problem.

I will try to work on it if nobody is faster than me :-)

Contributor

gforcada commented Jan 4, 2016

@tisto ok, thanks, so at least I'm right on diagnosing the problem.

I will try to work on it if nobody is faster than me :-)

@do3cc

This comment has been minimized.

Show comment
Hide comment
@do3cc

do3cc Jan 28, 2016

Member

Important progress update.
For now I kicked plone.app.folder and plone.namedfile from the tests because these packages use both ZopeTestCase AND plone.app.testing.
They can only work, if they choose ONE of those things. I am writing docs in plone.app.testing that explain why one has to choose

Member

do3cc commented Jan 28, 2016

Important progress update.
For now I kicked plone.app.folder and plone.namedfile from the tests because these packages use both ZopeTestCase AND plone.app.testing.
They can only work, if they choose ONE of those things. I am writing docs in plone.app.testing that explain why one has to choose

@do3cc

This comment has been minimized.

Show comment
Hide comment
@do3cc

do3cc Jan 28, 2016

Member

I am giving up here for now.
I started to kick out packages that use both plone.app.testing and ZopeTestCase.
Since more and more crept up i wrote a little script to find them simpler:

for pat in `cat bin/test | grep test-path | sed -e 's/.*\(.Users.*\).,.*/\1/'`
do
  echo $pat | rev | cut -d/ -f1,2 | rev
  ag 'ZopeTestCase|plone.app.testing' $pat | sed -Ee 's/.*(plone.app.testing|ZopeTestCase).*/\1/' | sort | uniq -c
done

Mac specific!

Without a real counting, just by looking, it seems that about 10% of packages use both systems. I can't just kick them all out :-( They all need to be converted to plone.app.testing, step by step.

Member

do3cc commented Jan 28, 2016

I am giving up here for now.
I started to kick out packages that use both plone.app.testing and ZopeTestCase.
Since more and more crept up i wrote a little script to find them simpler:

for pat in `cat bin/test | grep test-path | sed -e 's/.*\(.Users.*\).,.*/\1/'`
do
  echo $pat | rev | cut -d/ -f1,2 | rev
  ag 'ZopeTestCase|plone.app.testing' $pat | sed -Ee 's/.*(plone.app.testing|ZopeTestCase).*/\1/' | sort | uniq -c
done

Mac specific!

Without a real counting, just by looking, it seems that about 10% of packages use both systems. I can't just kick them all out :-( They all need to be converted to plone.app.testing, step by step.

@do3cc

This comment has been minimized.

Show comment
Hide comment
@do3cc

do3cc Jan 28, 2016

Member

The packages with problems and the script to find them. Only on mac

for pat in `cat bin/test | grep test-path | sed -e 's/.*\(.Users.*\).,.*/\1/' | sort | uniq`
do
  echo $pat | rev | cut -d/ -f1,2 | rev
  ag 'ZopeTestCase|plone.app.testing' $pat | sed -Ee 's/.*(plone.app.testing|ZopeTestCase).*/\1/' | sort | uniq -c
done | awk '/[0-9].*plone.app.testing/ {pat=1} /[0-9].*ZopeTestCase/ {ZTC=1} /src|egg/ {if (ZTC && pat) {print "- [ ] " last}; ZTC=0; pat=0; split($0, x1, "/"); split(x1[2], x2, "-"); last=x2[1]}'

@gforcada moved the list of packages on the first message, so that github show the nice progress bar

Member

do3cc commented Jan 28, 2016

The packages with problems and the script to find them. Only on mac

for pat in `cat bin/test | grep test-path | sed -e 's/.*\(.Users.*\).,.*/\1/' | sort | uniq`
do
  echo $pat | rev | cut -d/ -f1,2 | rev
  ag 'ZopeTestCase|plone.app.testing' $pat | sed -Ee 's/.*(plone.app.testing|ZopeTestCase).*/\1/' | sort | uniq -c
done | awk '/[0-9].*plone.app.testing/ {pat=1} /[0-9].*ZopeTestCase/ {ZTC=1} /src|egg/ {if (ZTC && pat) {print "- [ ] " last}; ZTC=0; pat=0; split($0, x1, "/"); split(x1[2], x2, "-"); last=x2[1]}'

@gforcada moved the list of packages on the first message, so that github show the nice progress bar

@gforcada gforcada changed the title from Bump zope.testrunner to Bump zope.testrunner / fix test isolation problems Jan 30, 2016

@gforcada gforcada referenced this pull request Jan 30, 2016

Closed

Get rid of ZopeTestCase #1348

@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Jan 30, 2016

Contributor

@do3cc could we add a command line switch to plone.app.testing to log somewhere which tests use ZopeTestCase? That would make things much easier to know what has to be fixed/changed, as it is not as simple as to grep through a package looking for ZopeTestCase.

plone.app.testing has a bbb module that uses ZopeTestCase...

Contributor

gforcada commented Jan 30, 2016

@do3cc could we add a command line switch to plone.app.testing to log somewhere which tests use ZopeTestCase? That would make things much easier to know what has to be fixed/changed, as it is not as simple as to grep through a package looking for ZopeTestCase.

plone.app.testing has a bbb module that uses ZopeTestCase...

@gforcada gforcada referenced this pull request Jan 30, 2016

Merged

Remove ZopeTestCase #1349

@do3cc

This comment has been minimized.

Show comment
Hide comment
@do3cc

do3cc Jan 30, 2016

Member

You mean the test runner itself, no?
Plone.app.testing does not know how triggered patching.

You can find out from the output of the testrunner. The patching happens in the zopelite Layer. So you look for setup ZopeLite and then for the next occurrence of Running...tests above that line

Von meinem iPhone gesendet

Am 30.01.2016 um 02:48 schrieb Gil Forcada Codinachs <notifications@github.commailto:notifications@github.com>:

@do3cchttps://github.com/do3cc could we add a command line switch to plone.app.testing to log somewhere which tests use ZopeTestCase? That would make things much easier to know what has to be fixed/changed, as it is not as simple as to grep through a package looking for ZopeTestCase.

plone.app.testing has a bbb modulehttps://github.com/plone/plone.app.testing/blob/master/plone/app/testing/bbb.py that uses ZopeTestCase...

Reply to this email directly or view it on GitHubhttps://github.com/plone/buildout.coredev/pull/155#issuecomment-177044675.

________________________________________________________ The contents of this e-mail and any attachments are confidential to the intended recipient. They may not be disclosed to or used by or copied in any way by anyone other than the intended recipient. If this e-mail is received in error, please immediately notify the sender and delete the e-mail and attached documents. Please note that neither the sender nor the sender's company accept any responsibility for viruses and it is your responsibility to scan or otherwise check this e-mail and any attachments.

Member

do3cc commented Jan 30, 2016

You mean the test runner itself, no?
Plone.app.testing does not know how triggered patching.

You can find out from the output of the testrunner. The patching happens in the zopelite Layer. So you look for setup ZopeLite and then for the next occurrence of Running...tests above that line

Von meinem iPhone gesendet

Am 30.01.2016 um 02:48 schrieb Gil Forcada Codinachs <notifications@github.commailto:notifications@github.com>:

@do3cchttps://github.com/do3cc could we add a command line switch to plone.app.testing to log somewhere which tests use ZopeTestCase? That would make things much easier to know what has to be fixed/changed, as it is not as simple as to grep through a package looking for ZopeTestCase.

plone.app.testing has a bbb modulehttps://github.com/plone/plone.app.testing/blob/master/plone/app/testing/bbb.py that uses ZopeTestCase...

Reply to this email directly or view it on GitHubhttps://github.com/plone/buildout.coredev/pull/155#issuecomment-177044675.

________________________________________________________ The contents of this e-mail and any attachments are confidential to the intended recipient. They may not be disclosed to or used by or copied in any way by anyone other than the intended recipient. If this e-mail is received in error, please immediately notify the sender and delete the e-mail and attached documents. Please note that neither the sender nor the sender's company accept any responsibility for viruses and it is your responsibility to scan or otherwise check this e-mail and any attachments.

@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Jan 30, 2016

Contributor

@do3cc yes sorry, you are totally right (writing at 3AM is probably not the best time :-), zope.testrunner should have this option.

Contributor

gforcada commented Jan 30, 2016

@do3cc yes sorry, you are totally right (writing at 3AM is probably not the best time :-), zope.testrunner should have this option.

@pbauer

This comment has been minimized.

Show comment
Hide comment
@pbauer

pbauer Feb 3, 2016

Member

The ValueError: undefined property 'content_meta_type'-Errors were due to the incomplete uninstall-profile of p.a.contenttypes that was run in tearDownPloneSite. I fixed that in plone/plone.app.contenttypes@34916e5 .

But still: A registered type should not leak to other tests.

Member

pbauer commented Feb 3, 2016

The ValueError: undefined property 'content_meta_type'-Errors were due to the incomplete uninstall-profile of p.a.contenttypes that was run in tearDownPloneSite. I fixed that in plone/plone.app.contenttypes@34916e5 .

But still: A registered type should not leak to other tests.

@do3cc

This comment has been minimized.

Show comment
Hide comment
@do3cc

do3cc Feb 3, 2016

Member

I guess some caching of stuff happens in GS

Member

do3cc commented Feb 3, 2016

I guess some caching of stuff happens in GS

gforcada added a commit to plone/plone.app.testing that referenced this pull request Feb 15, 2016

Remove bbb module
This will help identify all tests that rely on it.

Specially meant for plone/buildout.coredev#155
@gforcada

This comment has been minimized.

Show comment
Hide comment
@gforcada

gforcada Feb 15, 2016

Contributor

I just thought, that specially for the tests that are not directly importing/deriving from ZopeTestCase, but instead doing it through plone.app.testing.bbb module, this branch can be really helpful identifying all tests that need to be updated: plone/plone.app.testing@7259887

Contributor

gforcada commented Feb 15, 2016

I just thought, that specially for the tests that are not directly importing/deriving from ZopeTestCase, but instead doing it through plone.app.testing.bbb module, this branch can be really helpful identifying all tests that need to be updated: plone/plone.app.testing@7259887

mister-roboto pushed a commit that referenced this pull request Aug 16, 2017

[fc] Repository: icalendar
Branch: refs/heads/master
Date: 2017-07-18T11:58:09+02:00
Author: Christian Geier (geier) <geier@lostpackets.de>
Commit: collective/icalendar@9a03ff1

Allow ignoring of TZ offsets &gt; 24h

fixes #155

Files changed:
M CHANGES.rst
M src/icalendar/prop.py
M src/icalendar/tests/test_unit_cal.py
Repository: icalendar

Branch: refs/heads/master
Date: 2017-08-16T23:35:39+02:00
Author: Christian Geier (geier) <github@lostpackets.de>
Commit: collective/icalendar@68e33af

Merge pull request #235 from collective/fix/155

Allow ignoring of TZ offsets &gt; 24h

Files changed:
M CHANGES.rst
M src/icalendar/prop.py
M src/icalendar/tests/test_unit_cal.py

@gforcada gforcada reopened this Aug 17, 2017

@gforcada gforcada referenced this pull request Feb 3, 2018

Open

TRACKER: test isolation problems #2290

2 of 18 tasks complete

@gforcada gforcada closed this Feb 3, 2018

@jensens jensens deleted the testrunner branch Mar 29, 2018

mister-roboto pushed a commit that referenced this pull request Apr 9, 2018

[fc] Repository: plone.app.upgrade
Branch: refs/heads/master
Date: 2018-04-04T00:57:10+02:00
Author: Gil Forcada (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.upgrade@2dd2203

Update i18n domain for some actions

See plone/plone.app.event#204

Files changed:
A plone/app/upgrade/v51/final.py
M CHANGES.rst
M plone/app/upgrade/v51/configure.zcml
Repository: plone.app.upgrade

Branch: refs/heads/master
Date: 2018-04-09T10:59:32+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.upgrade@25ef84d

Merge pull request #155 from plone/gforcada-update-domain

Update i18n domain for some actions

Files changed:
A plone/app/upgrade/v51/final.py
M CHANGES.rst
M plone/app/upgrade/v51/configure.zcml

mister-roboto pushed a commit that referenced this pull request Apr 9, 2018

[fc] Repository: plone.app.upgrade
Branch: refs/heads/master
Date: 2018-04-04T00:57:10+02:00
Author: Gil Forcada (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.upgrade@2dd2203

Update i18n domain for some actions

See plone/plone.app.event#204

Files changed:
A plone/app/upgrade/v51/final.py
M CHANGES.rst
M plone/app/upgrade/v51/configure.zcml
Repository: plone.app.upgrade

Branch: refs/heads/master
Date: 2018-04-09T10:59:32+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.upgrade@25ef84d

Merge pull request #155 from plone/gforcada-update-domain

Update i18n domain for some actions

Files changed:
A plone/app/upgrade/v51/final.py
M CHANGES.rst
M plone/app/upgrade/v51/configure.zcml

mister-roboto pushed a commit that referenced this pull request Apr 9, 2018

[fc] Repository: plone.app.upgrade
Branch: refs/heads/master
Date: 2018-04-04T00:57:10+02:00
Author: Gil Forcada (gforcada) <gil.gnome@gmail.com>
Commit: plone/plone.app.upgrade@2dd2203

Update i18n domain for some actions

See plone/plone.app.event#204

Files changed:
A plone/app/upgrade/v51/final.py
M CHANGES.rst
M plone/app/upgrade/v51/configure.zcml
Repository: plone.app.upgrade

Branch: refs/heads/master
Date: 2018-04-09T10:59:32+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.upgrade@25ef84d

Merge pull request #155 from plone/gforcada-update-domain

Update i18n domain for some actions

Files changed:
A plone/app/upgrade/v51/final.py
M CHANGES.rst
M plone/app/upgrade/v51/configure.zcml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment