Re: [nft PATCH v4 2/8] tests: py: Enable JSON and JSON schema by default

[Phil Sutter <phil@nwl.cc>]

On Thu, Sep 04, 2025 at 06:56:08PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Sep 04, 2025 at 06:13:06PM +0200, Phil Sutter wrote:
> > On Thu, Sep 04, 2025 at 05:29:54PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Sep 04, 2025 at 05:24:48PM +0200, Phil Sutter wrote:
> > > > Introduce -J/--disable-json and -S/--no-schema to explicitly disable
> > > > them if desired.
> > > > 
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > ---
> > > >  tests/py/nft-test.py | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/tests/py/nft-test.py b/tests/py/nft-test.py
> > > > index 984f2b937a077..12c6174b01257 100755
> > > > --- a/tests/py/nft-test.py
> > > > +++ b/tests/py/nft-test.py
> > > > @@ -1488,7 +1488,11 @@ def set_delete_elements(set_element, set_name, table, filename=None,
> > > >  
> > > >      parser.add_argument('-j', '--enable-json', action='store_true',
> > > >                          dest='enable_json',
> > > > -                        help='test JSON functionality as well')
> > > > +                        help='test JSON functionality as well (default)')
> > > > +
> > > > +    parser.add_argument('-J', '--disable-json', action='store_true',
> > > > +                        dest='disable_json',
> > > > +                        help='Do not test JSON functionality as well')
> > > 
> > > Would it be possible to have common options to the different tests?
> > > 
> > > 1/8 uses -s and -j.
> > > 
> > > I am not sure we have to worry about breaking backward for test
> > > syntax, we only run this.
> > 
> > It's a bit of a mess with nft-test.py as it always performs standard
> > syntax testing and JSON syntax is an add-on one may enable (or not). So
> > to test JSON only, I'd have to refactor the ~300 lines long rule_add()
> > function. Not the worst thing to do, but much more work than "just"
> > having --enable-json being the default.
> 
> Oh, I see, so this is:
> 
> * no -j, then only standard is tested.
> * -j, both standard and json are tested.
> 
> Maybe more simple is to reverse this logic, ie.
> 
> * no -j, then both standard and json syntax are tested.
> * -s, only standard is tested.

So basically rename -J to -s in my patch? ;)
Note that -s clashes with --schema (although one could drop it as my
patch enables it by default, as well).

> Does this help?

Let me suggest an alternative: I promise to refactor that famous italian
pasta style function into smaller chunks like:

- Gather test data from files
- Instantiate a rule object with methods:
  - apply()
  - json_apply()
  - validate_payload()
  - validate_listing()
- Perform checks as per user choice (-j, -s, both/none)

And we live with the inconsistent UI for now?

Cheers, Phil