callback integration test: use difflib rather than comparing lines

[simonLeary42]

SUMMARY

Modified callback integration test tooling to use difflib rather than comparing lines. The existing approach struggles to compare strings with different numbers of lines.

While working on my own callback testing, I found the abominable jinja diff implementation, and came up with my own workflow using the diff action plugin return value. This paired nicely with my diff filter. Requiring ansible-test integration --diff is a bit clunky, though. I figured this small change would be more likely to be merged than my cool hack.

I see that the jinja diff has been rewritten in python (eff25c8). @felixfontein using difflib seems like a no brainer to me, did you consider this approach and decide against it?

As far as I can tell, difflib has been in the python standard library at least as far back as python 2.6: https://docs.python.org/2.6/library/difflib.html.

ISSUE TYPE

• Test Pull Request

COMPONENT NAME

callback

ADDITIONAL INFORMATION

before:

failed: [testhost] (item=Basic run) => {
    "ansible_loop_var": "result",
    "assertion": "result.output.differences | length == 0",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed",
    "result": {
        "name": "Basic run",
        "output": {
            "differences": [
                {
                    "line": {
                        "expected": "ok: [testhost] => ",
                        "got": "ok: [testhost] => None"
                    }
                },
                {
                    "line": {
                        "expected": "  msg: sample debug msg",
                        "got": ""
                    }
                },
                {
                    "line": {
                        "expected": "",
                        "got": "PLAY RECAP *********************************************************************"
                    }
                },
                {
                    "line": {
                        "expected": "PLAY RECAP *********************************************************************",
                        "got": "testhost                   : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   "
                    }
                },
                {
                    "line": {
                        "expected": "testhost                   : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   ",
                        "got": null
                    }
                }
            ],
            "expected": [
                "",
                "PLAY [testhost] ****************************************************************",
                "",
                "TASK [Sample task name] ********************************************************",
                "ok: [testhost] => ",
                "  msg: sample debug msg",
                "",
                "PLAY RECAP *********************************************************************",
                "testhost                   : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   "
            ],
            "got": [
                "",
                "PLAY [testhost] ****************************************************************",
                "",
                "TASK [Sample task name] ********************************************************",
                "ok: [testhost] => None",
                "",
                "PLAY RECAP *********************************************************************",
                "testhost                   : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   "
            ]
        }
    }
}

after:

failed: [testhost] (item=Basic run) => {
    "ansible_loop_var": "result",
    "assertion": "result.output.diff | length == 0",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed",
    "result": {
        "name": "Basic run",
        "output": {
            "diff": [
                "--- expected\n",
                "+++ got\n",
                "@@ -2,8 +2,7 @@\n",
                " PLAY [testhost] ****************************************************************",
                " ",
                " TASK [Sample task name] ********************************************************",
                "-ok: [testhost] => ",
                "-  msg: sample debug msg",
                "+ok: [testhost] => None",
                " ",
                " PLAY RECAP *********************************************************************",
                " testhost                   : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   "
            ],
            "expected": [
                "",
                "PLAY [testhost] ****************************************************************",
                "",
                "TASK [Sample task name] ********************************************************",
                "ok: [testhost] => ",
                "  msg: sample debug msg",
                "",
                "PLAY RECAP *********************************************************************",
                "testhost                   : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   "
            ],
            "got": [
                "",
                "PLAY [testhost] ****************************************************************",
                "",
                "TASK [Sample task name] ********************************************************",
                "ok: [testhost] => None",
                "",
                "PLAY RECAP *********************************************************************",
                "testhost                   : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   "
            ]
        }
    }
}

[felixfontein]

The main problem with difflib is that you cannot specify multiple variants of a line, I think. This is something we use in the tests, or at least were using (to work around differences in ansible-core versions): expected_output can be a list of strings or lists of strings. You're trying to remove this in tests/integration/targets/callback_diy/tasks/main.yml, but it can break (also for other callbacks) in any other place again, and thus this feature must be kept supported. If you can find a way to do that with difflib, that's fine, but if not, then we cannot use it.

[simonLeary42]

OK, that makes sense. One potential workaround would be allowing a test case to include parameters for 're.sub'. I used this to get rid of unpredictable output, such as the elapsed time from 'ansible.posix.profile_tasks'.

[felixfontein]

difflib.SequenceMatcher looks interesting, but it requires the list elements to be hashable (likely to get the runtime from cubic to quadratic). So it won't work here...

Regex support would also be fine (I'd do it additional to providing alternatives), but it also won't work for difflib...

I guess we really need an own diff implementation. The one used right now is quite primitive, but works. If you want to implement a more fancy one, feel free...

[russoz]

Changing the RV name from differences to diff is a breaking change. Right? Cc: @felixfontein

[felixfontein]

Yes, but that doesn't matter, since it's an internal plugin used only by the integration tests that cannot be used from anywhere else.

[simonLeary42]

it's an internal plugin used only by the integration tests that cannot be used from anywhere else.

expected_output can be a list of strings or lists of strings. ... this feature must be kept supported.

If I only had to remove one instance of this list-of-strings syntax, does that mean there are no other uses of this feature? If there is only currently one use of this feature, would it be the end of the world if it was superceded by regex preprocessing?

it requires the list elements to be hashable

Is the only non-hashable input the list of strings?

[simonLeary42]

[regex] also won't work for difflib...

Why is this?

[felixfontein]

If I only had to remove one instance of this list-of-strings syntax, does that mean there are no other uses of this feature? If there is only currently one use of this feature, would it be the end of the world if it was superceded by regex preprocessing?

There likely have been more in the past, and new needs can arise at any time, due to changes in ansible-core.

it requires the list elements to be hashable

Is the only non-hashable input the list of strings?

The problem isn't that lists of strings can't be hashed (you could convert them to a tuple of strings, which can), but that it uses the hash for equality checking. Probably it wants to be able to check whether an element is part of a set by using a regular Python set (or dict).

[regex] also won't work for difflib...

Why is this?

Due to how SequenceMatcher works: it requires list elements to be hashable, and needs elements that are equialent to result in the same hash.

[simonLeary42]

There likely have been more in the past, and new needs can arise at any time, due to changes in ansible-core.

Right, but I'm saying that those needs can be addressed with regex instead. If you want to allow one of 2 possible values for a line of text, you just write a regex that converts both of those possible values to a constant like "REPLACED", before doing the diff. Then you can use string hash for comparison.

[felixfontein]

That was my first idea as well, but I don't think it will work if you have multiple entries where you have multiple choices, and some of these choices coincide (while others do not).