Re: Unit testing dissector code

[João Valverde via Wireshark-dev <wireshark-dev () wireshark org>]

On 15/06/21 05:02, João Valverde via Wireshark-dev wrote:
> On 14/06/21 22:01, Martin Nyhus wrote:
> > On 05/06/2021 02:33, João Valverde wrote:
> > > But regarding your PoC having to give extern linkage to the internal
> > > dissector code is a big drawback IMO, even if it isn't visible in a DLL
> > > (because we use default hidden visibility when the compiler supports 
> > > it).
> > >
> > > Maybe that could be solved by including the dissector source file in the
> > > test source file?
> >
> > After a few different attempts I've updated the merge request with your
> > idea, and none of the changes to the dissector code are needed anymore.
> >
> > The new problem that shows up is that we now build the dissector twice,
> > once for the normal build and once when included in the test file, and
> > linking those fails because of the duplication. Unfortunately we can't
> > just exclude all the dissectors from the test linking as epan depends on
> > code in quite a few dissectors (enough that I gave up trying to make a
> > list of them).
>
> Having epan depend on dissectors shouldn't happen, although in practice 
> I think it has often been accepted as a mostly harmless expedient.
>
> It should be fixable. I'll take a look at that.

There are only 3 dependencies of epan on dissectors that I found, none 
that seemed too difficult to fix:
epan -> radius_dict -> radius
epan -> wslua -> dissect_tcp_pdus -> tcp
epan -> some dcerpc dependency I didn't investigate

So I built test-packet-bt-dht.c against epan minus dissectors, that 
worked, the first problem that came up is that BT-DHT wants to register 
on the udp tables and those don't exist. But the UDP table isn't 
necessary to run the test, the BT-DHT dissector is being called directly.

So either we're back to having to maintain tested/untested catalogs of 
dissectors and load every dissector with every test... Or we have to 
think up some other solution for this issue. Probably that solution is 
to be more tolerant of registration failures at runtime for test 
purposes...?

The next problem will be that a lot of dissectors won't work without a 
valid pinfo structure.

> > The solution I ended up with is to split out the dissectors that are
> > being tested, and making a new object that doesn't include those. In the
> > future if someone wants to add tests for a dissector that epan depends
> > on this won't work, but it should be compatible with the previous PoC of
> > header + non-static functions, so there are options available.
> >
> > If someone knows of a way to have Cmake include one object library in
> > another I'd like to get rid of DISSECTOR_UNTESTED, but for now I haven't
> > found a way to do it.
> >
> > Appreciate the feedback,
> > Martin
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives:    https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
            mailto:wireshark-dev-request () wireshark org?subject=unsubscribe