Change in vdsm[ovirt-3.5]: tests: disable memoization in testrunner

Hello Nir Soffer, Dan Kenigsberg,

I'd like you to do a code review.  Please visit

https://gerrit.ovirt.org/39949

to review the following change.

Change subject: tests: disable memoization in testrunner
......................................................................

tests: disable memoization in testrunner

memoization during tests is harmful, because
it adds another one hidden layer of global state
shared across test runs.

This may lead -and actually did- to hard
to debug failures in apparently unrelated code.

The net result is waste of developer's time,
and decreased trust in automated tests
because of mysterious failures.

To fix that, we now monkeypatch in testrunner
only utils.memoized with a fake which avoids caching.

Change-Id: Ibd986403a76268995b83c9e6ac400e942ed24cb6
Signed-off-by: Francesco Romani 
Reviewed-on: http://gerrit.ovirt.org/37748
Reviewed-by: Nir Soffer 
Tested-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
---
M tests/testrunner.py
1 file changed, 6 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/49/39949/1

diff --git a/tests/testrunner.py b/tests/testrunner.py
index 37b4c95..13fe55e 100644
--- a/tests/testrunner.py
+++ b/tests/testrunner.py
@@ -411,6 +411,10 @@
     raise AssertionError(msg)

+def fakeMemoized(func):
+    return func
+
+
 if __name__ == '__main__':
     if "--help" in sys.argv:
         print("testrunner options:\n"
@@ -422,4 +426,6 @@

# Mock panic() calls for tests
     utils.panic = panicMock
+    # Memoization during tests is a bad idea.
+    utils.memoized = fakeMemoized
     run()

[e-mail] has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 1: Verified-1

* Update tracker::IGNORE, no Bug-Url found

* Check Bug-Url::ERROR, At least one bug-url is required for the stable branch
* Check merged to previous::OK, change not open on any previous branch

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 1:

Why not backport the nicer fix we have in master?

Francesco Romani has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 1:

only because I have missed it. Will look again and abandon this patch.

Francesco Romani has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 1:

If I keep this patch, the later cleaner one is a quite trivial cherry-pick. Otherwise it needs changes.
I don't mind doing them, what do we prefer? stable branch as cleanest as possible or backports as closest to master patch as possible?

[e-mail] has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found

* Check Bug-Url::ERROR, At least one bug-url is required for the stable branch
* Check merged to previous::OK, change not open on any previous branch

[e-mail] has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 3:

* Update tracker::#1267851::OK
* Check Bug-Url::OK
* Check Public Bug::#1267851::OK, public bug
* Check Product::#1267851::OK, Correct classification oVirt
* Check TR::#1267851::ERROR, wrong target release for stable branch, --- should match ^3.[54321].*
* Check merged to previous::OK, change not open on any previous branch

Francesco Romani has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 3: Verified+1

tests passes, and this code will be used only in tests: V+1

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 3: Code-Review+1

Francesco Romani has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 3: Continuous-Integration+1

run CI checks manually

[e-mail] has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 4:

* Update tracker::#1267851::OK
* Check Bug-Url::OK
* Check Public Bug::#1267851::OK, public bug
* Check Product::#1267851::OK, Correct classification oVirt
* Check TR::#1267851::ERROR, wrong target release for stable branch, --- should match ^3.[54321].*
* Check merged to previous::OK, change not open on any previous branch

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 4: Code-Review+2

Francesco Romani has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 4: Continuous-Integration+1

Francesco Romani has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 4:

run CI scripts manually.

Francesco Romani has submitted this change and it was merged.

Change subject: tests: disable memoization in testrunner
......................................................................

tests: disable memoization in testrunner

memoization during tests is harmful, because
it adds another one hidden layer of global state
shared across test runs.

This may lead -and actually did- to hard
to debug failures in apparently unrelated code.

The net result is waste of developer's time,
and decreased trust in automated tests
because of mysterious failures.

To fix that, we now monkeypatch in testrunner
only utils.memoized with a fake which avoids caching.

Change-Id: Ibd986403a76268995b83c9e6ac400e942ed24cb6
Signed-off-by: Francesco Romani 
Bug-Url: https://bugzilla.redhat.com/1267851
Reviewed-on: http://gerrit.ovirt.org/37748
Reviewed-by: Nir Soffer 
Tested-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
Reviewed-on: https://gerrit.ovirt.org/39949
---
M tests/testrunner.py
1 file changed, 6 insertions(+), 0 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified; Passed CI tests

[e-mail] has posted comments on this change.

Change subject: tests: disable memoization in testrunner
......................................................................

Patch Set 5:

* Update tracker::#1267851::OK
* Set MODIFIED::bug 1267851::::#1267851::::IGNORE, not oVirt prod but vdsm