Re: [PATCH nft 1/1] tests/shell: sanitize "handle" in JSON output

[Phil Sutter <phil@nwl.cc>]

On Tue, Nov 21, 2023 at 01:58:41PM +0100, Thomas Haller wrote:
> On Tue, 2023-11-21 at 13:39 +0100, Phil Sutter wrote:
> > On Tue, Nov 21, 2023 at 01:10:11PM +0100, Thomas Haller wrote:
> > > On Sat, 2023-11-18 at 03:36 +0100, Phil Sutter wrote:
> > [...]
> > > > Also, scoping these replacements to line 1 is funny with single
> > > > line
> > > > input. Worse is identifying the change in the resulting diff.
> > > > Maybe
> > > > write a helper in python which lets you more comfortably sanitize
> > > > input,
> > > > sort attributes by key and output pretty-printed?
> > > 
> > > You mean, to parse and re-encode the JSON? That introduces
> > > additional
> > > changes, which seems undesirable. That's why the regex is limited
> > > to
> > > the first line (even if we only expect to ever see one line there).
> > > 
> > > Also, normalization via 2 regex seems simpler than writing some
> > > python.
> > > 
> > > Well, pretty-printing the output with `jq` would have the
> > > advantage,
> > > that future diffs might be smaller (changing individual lines, vs.
> > > replace one large line). Still, I think it's better to keep the
> > > amount
> > > of post-processing minimal.
> > 
> > The testsuite relies upon Python and respective modules already,
> > using
> > jq introduces a new dependency. Hence why I suggested to write a
> > script.
> > 
> > JSON object attributes are not bound to any ordering, the code may
> > change it.
> 
> Don't have .nft dumps the same concern?

Not as far as I can tell: Objects are sorted by name, rule ordering is
inherently relevant.

> In JSON the order of things certainly matters. libjansson has
> JSON_PRESERVE_ORDER, which is used by libnftables. Also,
> JSON_PRESERVE_ORDER is deprecated since 2016 and order is always
> preserved.

The reason why JSON_PRESERVE_ORDER exists is just because ordering does
not matter per se. For a proper JSON parser,
| {"a": 1, "b": 2}
and
| {"b": 2, "a": 1}
are semantically identical.

> If the order changes, that should be visible (in form of a test
> failure).

Why? If we change e.g. the ordering of array elements by adding them in
reverse, isn't this a legal change and any testsuite complaints about it
just noise?

> > When analyzing testsuite failures, a diff of two overlong lines is
> > inconvenient to the point that one may pipe both through json_pp and
> > then diff again. The testsuite may do just that in case of offending
> > output, but the problem of reordered attributes remains.
> > 
> > I'd really appreciate if testsuite changes prioritized usability. I
> > rather focus on fixing bugs instead of parsing the testsuite results.
> 
> The test suite prioritizes usability. No need to suggest otherwise.

Then why not store JSON dumps pretty printed to make diffs more
readable?

> To make debugging easier, the test suite can additionally show a
> prettified diff. It does not determine how the .json-nft file is stored
> in git. 

Is this "can" in a pending patch? Because I don't see that "prettified
diff" option in tests/shell/helpers/test-wrapper.sh.

Cheers, Phil