Revert "zodbpickle: v↑ (1.0.4 -> 2.0.0)"

This reverts commit 1c2d1c0a.
because NEO is not ready for such change.

software/neoppod/software-common.cfg

@@ -151,7 +151,7 @@ pycurl = 7.43.0
 setproctitle = 1.1.10
 slapos.recipe.template = 4.4
 transaction = 1.7.0
-zodbpickle = 2.0.0
+zodbpickle = 1.0.4
 zodbtools = 0.0.0.dev4
 cython-zstd = 0.2
 python-dateutil = 2.7.3

[Kirill Smelkov]

@jm, any details?

[Kirill Smelkov]

Answering to myself: to see what is wrong @romain suggested to run my patch wrt all NEO testsuites and see what breaks. This way I ran NEO.UnitTest-Master, NEO.UnitTest-Master.ZODB5, NEO.UnitTest-Master.ZODB3, and NEO.UnitTest-Master.PyPy wrt slapos with my patch. The results I got are:

NEO.UnitTest-Master-kirr: all PASS
NEO.UnitTest-Master.ZODB5-kirr: all PASS
NEO.UnitTest-Master.ZODB3-kirr: FAIL
NEO.UnitTest-Master.PyPy-kirr: all PASS

I looked inside ZODB3-related failures (as I too looked at failures when verifying my patch when pushing to @erp-component branch and looking into NEO failures after merging my patch to slapos master), and they are exactly the same failures that are already there failing for NEO with slapos without my patch:

NEO.UnitTest-Master.ZODB3-kirr / SQLite failed with:

FAIL: testBackupDelayedUnlockTransaction (neo.tests.threaded.testSSL.SSLReplicationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/slapgrid/slappart5/srv/testnode/dgb/soft/68fc043aba2ab2e43c30b3d4a422041a/parts/neoppod-repository/neo/tests/threaded/testReplication.py", line 56, in wrapper
    wrapped(self, backup)
  File "/srv/slapgrid/slappart5/srv/testnode/dgb/soft/68fc043aba2ab2e43c30b3d4a422041a/parts/neoppod-repository/neo/tests/threaded/testReplication.py", line 332, in testBackupDelayedUnlockTransaction
    self.assertTrue(msg.endswith('lag=ε'), msg)
failureException: RUNNING; UP_TO_DATE=1; ltid=03db4f6382d99699 (2020-10-15 13:55:30.667958)

neo_5
    BACKINGUP; UP_TO_DATE=1; ltid=03db4f6382d99698; lag=1e-06

Here is the same failure on NEO.UnitTest-Master.ZODB3 / MySQL from October.05.

NEO.UnitTest-Master.ZODB3-kirr / MySQL failed with

FAIL: testReplicationBlockedByUnfinished1 (neo.tests.threaded.testReplication.ReplicationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/slapgrid/slappart5/srv/testnode/dgb/soft/68fc043aba2ab2e43c30b3d4a422041a/parts/neoppod-repository/neo/tests/threaded/__init__.py", line 1243, in wrapper
    return wrapped(self, cluster, *args, **kw)
  File "/srv/slapgrid/slappart5/srv/testnode/dgb/soft/68fc043aba2ab2e43c30b3d4a422041a/parts/neoppod-repository/neo/tests/threaded/testReplication.py", line 1000, in testReplicationBlockedByUnfinished1
    self.assertPartitionTable(cluster, expected)
  File "/srv/slapgrid/slappart5/srv/testnode/dgb/soft/68fc043aba2ab2e43c30b3d4a422041a/parts/neoppod-repository/neo/tests/threaded/__init__.py", line 1146, in assertPartitionTable
    lambda x: index(x.getUUID()))
  File "/srv/slapgrid/slappart5/srv/testnode/dgb/soft/68fc043aba2ab2e43c30b3d4a422041a/parts/neoppod-repository/neo/tests/__init__.py", line 306, in assertPartitionTable
    '|'.join(pt._formatRows(sorted(pt.count_dict, key=key))))
  File "/srv/slapgrid/slappart5/srv/testnode/dgb/soft/68fc043aba2ab2e43c30b3d4a422041a/parts/neoppod-repository/neo/tests/__init__.py", line 301, in assertEqual
    return super(NeoTestBase, self).assertEqual(first, second, msg=msg)
failureException: 'UU|UU' != 'UO|UO'

Here is the same failure on NEO.UnitTest-Master / MySQL from October 13.

Given that in my run it was only ZODB3 related breakage and other test sutes were all PASSED, and that the failures inside ZODB3 were not new, I decided that my patch does not do harm and should be ok to be merged into master. Isn't it?

---

Now @romain is pointing me at https://nexedijs.erp5.net/#/test_result_module/20201014-364B99CB/3 where things break with TypeError: can't set attributes of built-in/extension type 'zodbpickle.binary' on binary._pack = bytes.__str__. This is indeed relevant to zodbpickle upgrade, but if one looks into TestResult that contains this TestResultLine everything is marked as PASSED:

https://nexedijs.erp5.net/#/test_result_module/20201014-364B99CB

The same is actually true for my runs, for example as said above NEO.UnitTest-Master-kirr says MySQL=PASSED and SQLite=PASSED in green, but if one looks inside the testlines it shows the TypeError failures:

https://erp5.nexedi.net/test_result_module/20201015-2AAD0EF4/2
https://erp5.nexedi.net/test_result_module/20201015-2AAD0EF4/3

---

So my question is:

is it expected not to trust reported "PASSED" status and manually look every time inside of every test run to be sure?

Personally I believe "PASSED" must be trusted, but this days I won't be surprised by anything...

For the reference: if PASSED must be trusted, the bug is in runTestSuite.in of NEO software release, and, in my view comes from duplication of many of those runTestSuite.in in between many places without single point where to fix bugs and improve (see d68874fa, !839 (merged)) /cc @jerome

[Romain Courteaud]

is it expected not to trust reported "PASSED" status and manually look every time inside of every test run to be sure?

There is a bug currently which mark test line without any result as PASSED.

Quickly looking at the test_result bt5, I guess the issue is in this script, which does not report 0 test count as a failure.

I applied this patch on Nexedi ERP5 to see if it helps. Please rerun your tests to see if it helps.

[Kirill Smelkov]

@romain, thanks, let's see if it helps indeed.

[Kirill Smelkov]

@romain, here is how now test result looks for my zodbpickle v↑ patch: https://erp5.nexedi.net/test_result_module/20201016-70D7B565 (result=UNKNOWN). This test result is also displayed in a list of test results with result=FAIL. In other words your change should, hopefully, prevent misinterpretations similar to the above in the future. Thanks.

[Jérome Perrin]

One thing I observed in the past is that when tested software can not be installed at all, the test is also marked as "PASSED". This might be for same reasons.

@romain romain/erp5@3e513f34 looks a good fix, would you mind adding a quick test in testTaskDistribution and committing the change ?

---

maybe test can be something like this:

  def test_???(self):
    test_result_path, _ = self._createTestResult(test_list=['testFoo', ])
    test_result = self.portal.unrestrictedTraverse(test_result_path)
    line_url, _ = self.tool.startUnitTest(test_result_path)
    test_result_line = self.portal.restrictedTraverse(line_url)
    status_dict = {
        'test_count': 0,
        'error_count': 0,
        'failure_count': 0,
    }
    self.tool.stopUnitTest(line_url, status_dict)
    self.tic()
    self.assertEqual(test_result_line.getStringIndex(), 'FAILED')
    self.assertEqual(test_result_line.getSimulationState(), 'stopped')
    self.assertEqual(test_result.getStringIndex(), 'FAIL')
    self.assertEqual(test_result.getSimulationState(), 'stopped')

[Romain Courteaud]

Here it is