Enable test reruns on failed fragiled tests

[avtikhon]

Added ability to use fragile list in JSON format from 'suite.ini'
files.

   fragile = {
        "retries": 10,
        "tests": {
            "bitset.test.lua": {
                "issues": [ "gh-4095" ],
                "checksums": [ "050af3a99561a724013995668a4bc71c", "f34be60193cfe9221d3fe50df657e9d3" ]
            }
        }}

Added ability to set per suite in suite.ini configuration file
'retries' option, which sets the number of accepted reruns of
the tests failed from 'fragile' list.

Added ability to check failed tests w/ fragile list to be sure that
the current fail equal to the issue mentioned in the fragile list.

Closes tarantool/tarantool#189.

[LeonidVas]

It looks like this closes # 189. Add this information to the commit message instead of "Part of tarantool / tarantool problem # 5050".

[avtikhon]

It looks like this closes # 189. Add this information to the commit message instead of "Part of tarantool / tarantool problem # 5050".

Ok, changed.

[LeonidVas]

After some discussion with @Totktonada and @avtikhon, it was decided to try using JSON for configuration a "fragile".
Example:

fragile =
 {
   "tests": {
     "test_name": {
       "checksums": [
         ch1,
         ch2
       ]
     }
   },
   "retries": 10
 }

Useful links:
https://docs.python.org/3/library/configparser.html#supported-ini-file-structure
https://tools.ietf.org/html/rfc7159

[avtikhon]

After some discussion with @Totktonada and @avtikhon, it was decided to try using JSON for configuration a "fragile".
Example:

fragile =
 {
   "tests": {
     "test_name": {
       "checksums": [
         ch1,
         ch2
       ]
     }
   },
   "retries": 10
 }

Useful links:
https://docs.python.org/3/library/configparser.html#supported-ini-file-structure
https://tools.ietf.org/html/rfc7159

Implemented.

[LeonidVas]

As I said before, transition to JSON format should be implemented by a first patch in the patchset. And all new "fragile" options need to be added only to JSON. Introducing two new competing formats in the same patchset is very strange decision.
Also, I think the format fragile = <basename of the test> ; gh-<issue> md5sum:<checksum> is strange (and potentially not valid), because:

• ; / # is used for a comment adding and it looks like md5sum: <checksum> is a comment, but it is not.
• = / : is a separator for key/value and mixing them in the same file is a strange decision.

[Totktonada]

I didn't look thoroughly, but I would highlight several points.

1. The approach with checksums is not universal. It does not work if time-related information appears in a result file. It requires periodical checksums updating if a line number is reported as part of an error. I guess it is okay to operate on a test level and either mark it as stable or as fragile.
2. We can either implement reruns within a worker or on the level upward: within the task management system (dispathers-listeners-workers). Since there is nothing tarantool-specific I would look into the latter way. I pushed a variant to Totktonada/rerun-fragile-test branch (0292fe2).

---

If you want my opinion about extracting checksums from comments — it is disgusting. Sorry.

There are more points I would highlight about the patchset, but it does not have much sense if we'll get rid of checksums at all. The patchset modifies many different parts of test-run: too much for this small feature, IMHO.

[LeonidVas]

Looks syntactically correct to me.

[Totktonada]

I pushed two branches into tarantool today (and rerun failed jobs several times):

• Got several 'address already in use' fails (http_client.test.lua). It is the problem of test-run design (Fix 'Address already in use' error with TCP sockets #141), not the test per se.
• Got several fails on net.box_reconnect_after_gh-3164.test.lua with the whole box.info.replication output (time related information, UUIDs).
• Got several fails on net.box_reconnect_after_gh-3164.test.lua with peer UUID, Unix socket path, schema version and so on (will differ from run to run).

There are other fails, but I didn't look whether the output was predictable (at least vinyl.ddl.test.lua, don't remember others).

I know, you're sure you can mark almost everything with checksums, but I see the opposite picture now. Maybe I'm just lucky.

At least, please, left ability to rerun a test, which is not marked with any checksum (I didn't look at the last patchset, maybe it is there already).

Aside of this, the 'transient fail' statistics was nice: no need to roll a screen to find whether reruns occur after full local testing.

[avtikhon]

I pushed two branches into tarantool today (and rerun failed jobs several times):

I've run the same 3 days twice in hour and I got really lot of results. Checked some of them I've got the same results as Alexander below, but it was expected. I've updated the table [1] with the new column 'Checksum' and set there some of results, green - checksums that available, red - that have changing data. As I've checked before the same issue with checksum we'll have just in some tests - found only 9 with printing box info and some other tests printing other changing data.

[1] - https://docs.google.com/spreadsheets/d/1LGfHM_6tn84lzlz3XIXQnOqNY_KtzRWuZg33_yuv4YI/edit#gid=1890598391

• Got several 'address already in use' fails (http_client.test.lua). It is the problem of test-run design (Fix 'Address already in use' error with TCP sockets #141), not the test per se.

Right, we have some these fails, but it passes when tests run inline using fragile list for it.

• Got several fails on net.box_reconnect_after_gh-3164.test.lua with the whole box.info.replication output (time related information, UUIDs).
• Got several fails on net.box_reconnect_after_gh-3164.test.lua with peer UUID, Unix socket path, schema version and so on (will differ from run to run).

Right this test I've in the table marked as red.

There are other fails, but I didn't look whether the output was predictable (at least vinyl.ddl.test.lua, don't remember others).

I know, you're sure you can mark almost everything with checksums, but I see the opposite picture now. Maybe I'm just lucky.

Right, we have some tests like this, but we have 2 ways to resolve it: fix the test either remove box information printing if it is not really needed. I've prepared the initial patch with checksums set as green in the table mentioned above and the results really good - only 1 new real fail found [2] !

[2] - https://gitlab.com/tarantool/tarantool/-/pipelines/193120097/builds

At least, please, left ability to rerun a test, which is not marked with any checksum (I didn't look at the last patchset, maybe it is there already).

That was the main cause why this feature with tests reruns I didn't want to have without checksums.

Aside of this, the 'transient fail' statistics was nice: no need to roll a screen to find whether reruns occur after full local testing.

Actually after checksums will be written to the suites.ini files these fails would be really rare.

[Totktonada]

Got several 'address already in use' fails (http_client.test.lua). It is the problem of test-run design (#141), not the test per se.

Right, we have some these fails, but it passes when tests run inline using fragile list for it.

It is maybe okay as a temporary workaround, but it is not so complex to fix the cause: bind httpd.py on zero port (to let kernel to choose a random one) and read a real port from its output (we already read heartbeats from here).

At least, please, left ability to rerun a test, which is not marked with any checksum (I didn't look at the last patchset, maybe it is there already).

That was the main cause why this feature with tests reruns I didn't want to have without checksums.

To be honest, I don't get the answer.

Aside of this, the 'transient fail' statistics was nice: no need to roll a screen to find whether reruns occur after full local testing.

Actually after checksums will be written to the suites.ini files these fails would be really rare.

But I still want to know about this, when it occurs, to verify whether it is introduced by a patch I work on. It is okay to postpone this feature, but it would be usable.

[Totktonada]

Here you block ability to rerun a test without a checksum. It is possible that a test cannot be verified just using a result file, but we want to allow reruns for it.

I remember the reason why you don't want to enable reruns by default when no checksums are provided: because it means that existing configs will let tests that are currently in a fragile list being retried.

However this is not so until we'll set retries explicitly, so it seems we can do this without subtle behaviour changes. I mean, if we'll consider empty (or non-existing) checksums list as 'rerun it anyway', everything will be good. But if you prefer explicit '*' in checksum, I don't mind.

You can also consider this as out of scope (not needed right now), but for me it looks quite natural that…

fragile = {
    "retries": 2,
    "tests": {
        "bitset.test.lua": {
            "issues": ["gh-4095"]
        }
    }
}

…means 'rerun bitset.test.lua two times'.

Anyway, it is up to you. I just noted that the problem you highlighted looks as not applicable to our situation.

[avtikhon]

I've commented it in the same time when you wrote the questions in here:
#217 (comment)
in short: I agree with it, but as standalone commit for this feature.

[Totktonada]

Status: It looks almost ready (considering given decisions), I just want to discuss several moments before we'll land the patchset.

[avtikhon]

Got several 'address already in use' fails (http_client.test.lua). It is the problem of test-run design (#141), not the test per se.

Right, we have some these fails, but it passes when tests run inline using fragile list for it.

It is maybe okay as a temporary workaround, but it is not so complex to fix the cause: bind httpd.py on zero port (to let kernel to choose a random one) and read a real port from its output (we already read heartbeats from here).

Absolutely agree that run the tests like this is temporary workaround and it need to be fixed. And we'll continue work on its fixing in already exists standalone issues like:

tarantool/tarantool-qa#228

At least, please, left ability to rerun a test, which is not marked with any checksum (I didn't look at the last patchset, maybe it is there already).

That was the main cause why this feature with tests reruns I didn't want to have without checksums.

To be honest, I don't get the answer.

I mean that blind rerun of the flaky issues is more dangerous than it's manual rerun that we currently have. Check of 'checksums' is a way to make these reruns partly automate to help the testing infrastructure to avoid of manual reruns. Standalone rerun we'll hide the new flaky issues.
Right, we can add the new feature to rerun the needed tests. It can be provided with special checksum/mark like:
"checksum": [ "temporary_mark_to_rerun_on_any_fail_NOT_TO_COMMIT" ]
and add the special checker in test-run scripts on this mark to pass the tests on rerun on any failed checksum. It will help to check manually some flaky tests. But I think this feature must be committed in separate patch from the current patchset with standalone issue number.
Also in this way I would suggest to have new feature as Sergey Bronnikov suggested to have in some testing mode mandatory reruns for all tests to be able to find the flaky tests at the very start of its appearing, but this feature I would like to implement as standalone commit too.

Aside of this, the 'transient fail' statistics was nice: no need to roll a screen to find whether reruns occur after full local testing.

Actually after checksums will be written to the suites.ini files these fails would be really rare.

But I still want to know about this, when it occurs, to verify whether it is introduced by a patch I work on. It is okay to postpone this feature, but it would be usable.

Right, let's implement it as a next step, not now.

[Totktonada]

LGTM.

[Totktonada]

The test-run submodule is updated in tarantool in the following commits: 2.6.0-104-g43482eedc, 2.5.1-86-gc5bb549f6, 2.4.2-70-gef330c3b0, 1.10.7-35-g91260069c.