(unit) testing of lwip

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

(unit) testing of lwip

Mate Varga
Hello,

I have made a small bug fix for lwip (#58473), and as a follow-up I'd like to make sure that this simple, compact functionality (converting hostname into a QNAME field) is unit tested. I have written a test and I'm trying to run the test suite, but it looks to me that the suite is broken in various ways (building it under *nix + gcc doesn't work, I can get into details later about how exactly).

I can try and fix the tests, but first I'd like to understand whether it is the intention of the project owners to keep the suite and the examples maintained -- I don't want to submit a patch and find out that it's not welcome :). Also it looks a bit odd that lwip's unit tests are run from and depend on lwip_contrib.

Should I invest effort into making some refactoring here? First of all I'd probably try to run unit tests without requiring lwip_contrib.

Thanks,
Mate

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: (unit) testing of lwip

goldsimon@gmx.de


Am 1. Juni 2020 11:37:28 MESZ schrieb Mate Varga <[hidden email]>:
>Hello,
>
>I have made a small bug fix for lwip (#58473), and as a follow-up I'd
>like to make sure that this simple, compact functionality (converting
>hostname into a QNAME field) is unit tested. I have written a test and
>I'm trying to run the test suite, but it looks to me that the suite is
>broken in various ways (building it under *nix + gcc doesn't work, I
>can get into details later about how exactly).

It works for me...

>
>I can try and fix the tests, but first I'd like to understand whether
>it is the intention of the project owners to keep the suite and the
>examples maintained -- I don't want to submit a patch and find out that
>it's not welcome :). Also it looks a bit odd that lwip's unit tests are
>run from and depend on lwip_contrib.

There's no "lwip_contrib" any more. The files are integrated into the mail repo now. I guess you're not using latest git master?

Regards,
Simon

>
>Should I invest effort into making some refactoring here? First of all
>I'd probably try to run unit tests without requiring lwip_contrib.
>
>Thanks,
>Mate

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: (unit) testing of lwip

Mate Varga
Ah okay, thanks. I have probably read some old documentation about who to run the test suite. I'll submit the other changeset then soon.

On Mon, Jun 1, 2020, at 11:45, goldsimon wrote:

>
>
> Am 1. Juni 2020 11:37:28 MESZ schrieb Mate Varga <[hidden email]>:
> >Hello,
> >
> >I have made a small bug fix for lwip (#58473), and as a follow-up I'd
> >like to make sure that this simple, compact functionality (converting
> >hostname into a QNAME field) is unit tested. I have written a test and
> >I'm trying to run the test suite, but it looks to me that the suite is
> >broken in various ways (building it under *nix + gcc doesn't work, I
> >can get into details later about how exactly).
>
> It works for me...
>
> >
> >I can try and fix the tests, but first I'd like to understand whether
> >it is the intention of the project owners to keep the suite and the
> >examples maintained -- I don't want to submit a patch and find out that
> >it's not welcome :). Also it looks a bit odd that lwip's unit tests are
> >run from and depend on lwip_contrib.
>
> There's no "lwip_contrib" any more. The files are integrated into the
> mail repo now. I guess you're not using latest git master?
>
> Regards,
> Simon
>
> >
> >Should I invest effort into making some refactoring here? First of all
> >I'd probably try to run unit tests without requiring lwip_contrib.
> >
> >Thanks,
> >Mate
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: (unit) testing of lwip

Mate Varga
Hi,

How should I approach DNS-related testing? The specific change I'd like to make (feedback welcome): in dns.c, the dns_send() function splits the hostname into multiple parts and populates the packet buffer's QNAME field. This has/had a bug that is trivial to fix (see patch on Savannah), but in order to test it, I need to extract this functionality into a separate function (that can be tested in isolation), something like

/**
 * Convert hostname into suitable query format
 * and populate QNAME region of the packet buffer
 *
 * @param hostname the target host's name
 * @param pre-allocated p buffer for the outbound UDP packet
 * @return position of the end of the QNAME region
 */
err_t
dns_convert_hostname(const char * hostname, struct pbuf *p, u16_t *query_idx);

My problem is that the the default test suite has LWIP_DNS undefined (well, def'd to 0), therefore dns.c is not included in the build.

What do you recommend?
1) compile the DNS code when running unit tests (define LWIP_DNS and iron out issues like that pbuf and mem unit tests require LWIP_DNS to be falsy)
2) move this utility method out from dns.c
3) something else

Thanks,
Mate

On Mon, Jun 1, 2020, at 11:51, Mate Varga wrote:

> Ah okay, thanks. I have probably read some old documentation about who
> to run the test suite. I'll submit the other changeset then soon.
>
> On Mon, Jun 1, 2020, at 11:45, goldsimon wrote:
> >
> >
> > Am 1. Juni 2020 11:37:28 MESZ schrieb Mate Varga <[hidden email]>:
> > >Hello,
> > >
> > >I have made a small bug fix for lwip (#58473), and as a follow-up I'd
> > >like to make sure that this simple, compact functionality (converting
> > >hostname into a QNAME field) is unit tested. I have written a test and
> > >I'm trying to run the test suite, but it looks to me that the suite is
> > >broken in various ways (building it under *nix + gcc doesn't work, I
> > >can get into details later about how exactly).
> >
> > It works for me...
> >
> > >
> > >I can try and fix the tests, but first I'd like to understand whether
> > >it is the intention of the project owners to keep the suite and the
> > >examples maintained -- I don't want to submit a patch and find out that
> > >it's not welcome :). Also it looks a bit odd that lwip's unit tests are
> > >run from and depend on lwip_contrib.
> >
> > There's no "lwip_contrib" any more. The files are integrated into the
> > mail repo now. I guess you're not using latest git master?
> >
> > Regards,
> > Simon
> >
> > >
> > >Should I invest effort into making some refactoring here? First of all
> > >I'd probably try to run unit tests without requiring lwip_contrib.
> > >
> > >Thanks,
> > >Mate
> >
> > _______________________________________________
> > lwip-devel mailing list
> > [hidden email]
> > https://lists.nongnu.org/mailman/listinfo/lwip-devel
> >
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: (unit) testing of lwip

Erik Ekman
Hi

I recently enabled DNS in the unit tests and added a simple first testcase,
so it should be possible to add more now.

What do you think about also removing (or ignoring) the extra dot earlier?
It would be good if lookups of example.com. and example.com would share the same dns_table_entry.

/Erik


On Tue, 2 Jun 2020 at 11:47, Mate Varga <[hidden email]> wrote:
Hi,

How should I approach DNS-related testing? The specific change I'd like to make (feedback welcome): in dns.c, the dns_send() function splits the hostname into multiple parts and populates the packet buffer's QNAME field. This has/had a bug that is trivial to fix (see patch on Savannah), but in order to test it, I need to extract this functionality into a separate function (that can be tested in isolation), something like

/**
 * Convert hostname into suitable query format
 * and populate QNAME region of the packet buffer
 *
 * @param hostname the target host's name
 * @param pre-allocated p buffer for the outbound UDP packet
 * @return position of the end of the QNAME region
 */
err_t
dns_convert_hostname(const char * hostname, struct pbuf *p, u16_t *query_idx);

My problem is that the the default test suite has LWIP_DNS undefined (well, def'd to 0), therefore dns.c is not included in the build.

What do you recommend?
1) compile the DNS code when running unit tests (define LWIP_DNS and iron out issues like that pbuf and mem unit tests require LWIP_DNS to be falsy)
2) move this utility method out from dns.c
3) something else

Thanks,
Mate

On Mon, Jun 1, 2020, at 11:51, Mate Varga wrote:
> Ah okay, thanks. I have probably read some old documentation about who
> to run the test suite. I'll submit the other changeset then soon.
>
> On Mon, Jun 1, 2020, at 11:45, goldsimon wrote:
> >
> >
> > Am 1. Juni 2020 11:37:28 MESZ schrieb Mate Varga <[hidden email]>:
> > >Hello,
> > >
> > >I have made a small bug fix for lwip (#58473), and as a follow-up I'd
> > >like to make sure that this simple, compact functionality (converting
> > >hostname into a QNAME field) is unit tested. I have written a test and
> > >I'm trying to run the test suite, but it looks to me that the suite is
> > >broken in various ways (building it under *nix + gcc doesn't work, I
> > >can get into details later about how exactly).
> >
> > It works for me...
> >
> > >
> > >I can try and fix the tests, but first I'd like to understand whether
> > >it is the intention of the project owners to keep the suite and the
> > >examples maintained -- I don't want to submit a patch and find out that
> > >it's not welcome :). Also it looks a bit odd that lwip's unit tests are
> > >run from and depend on lwip_contrib.
> >
> > There's no "lwip_contrib" any more. The files are integrated into the
> > mail repo now. I guess you're not using latest git master?
> >
> > Regards,
> > Simon
> >
> > >
> > >Should I invest effort into making some refactoring here? First of all
> > >I'd probably try to run unit tests without requiring lwip_contrib.
> > >
> > >Thanks,
> > >Mate
> >
> > _______________________________________________
> > lwip-devel mailing list
> > [hidden email]
> > https://lists.nongnu.org/mailman/listinfo/lwip-devel
> >
>
> _______________________________________________
> lwip-devel mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/lwip-devel
>

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel

_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

Re: (unit) testing of lwip

Mate Varga

Thanks, I'll check it out later this week. Your suggestion makes sense, I'll try that.

M.
On Tue, Jul 7, 2020, at 16:14, Erik Ekman wrote:
Hi

I recently enabled DNS in the unit tests and added a simple first testcase,
so it should be possible to add more now.

What do you think about also removing (or ignoring) the extra dot earlier?
It would be good if lookups of example.com. and example.com would share the same dns_table_entry.

/Erik


On Tue, 2 Jun 2020 at 11:47, Mate Varga <[hidden email]> wrote:
Hi,

How should I approach DNS-related testing? The specific change I'd like to make (feedback welcome): in dns.c, the dns_send() function splits the hostname into multiple parts and populates the packet buffer's QNAME field. This has/had a bug that is trivial to fix (see patch on Savannah), but in order to test it, I need to extract this functionality into a separate function (that can be tested in isolation), something like

/**
 * Convert hostname into suitable query format
 * and populate QNAME region of the packet buffer
 *
 * @param hostname the target host's name
 * @param pre-allocated p buffer for the outbound UDP packet
 * @return position of the end of the QNAME region
 */
err_t
dns_convert_hostname(const char * hostname, struct pbuf *p, u16_t *query_idx);

My problem is that the the default test suite has LWIP_DNS undefined (well, def'd to 0), therefore dns.c is not included in the build.

What do you recommend?
1) compile the DNS code when running unit tests (define LWIP_DNS and iron out issues like that pbuf and mem unit tests require LWIP_DNS to be falsy)
2) move this utility method out from dns.c
3) something else

Thanks,
Mate

On Mon, Jun 1, 2020, at 11:51, Mate Varga wrote:
> Ah okay, thanks. I have probably read some old documentation about who
> to run the test suite. I'll submit the other changeset then soon.
>
> On Mon, Jun 1, 2020, at 11:45, goldsimon wrote:
> >
> >
> > Am 1. Juni 2020 11:37:28 MESZ schrieb Mate Varga <[hidden email]>:
> > >Hello,
> > >
> > >I have made a small bug fix for lwip (#58473), and as a follow-up I'd
> > >like to make sure that this simple, compact functionality (converting
> > >hostname into a QNAME field) is unit tested. I have written a test and
> > >I'm trying to run the test suite, but it looks to me that the suite is
> > >broken in various ways (building it under *nix + gcc doesn't work, I
> > >can get into details later about how exactly).
> >
> > It works for me...
> >
> > >
> > >I can try and fix the tests, but first I'd like to understand whether
> > >it is the intention of the project owners to keep the suite and the
> > >examples maintained -- I don't want to submit a patch and find out that
> > >it's not welcome :). Also it looks a bit odd that lwip's unit tests are
> > >run from and depend on lwip_contrib.
> >
> > There's no "lwip_contrib" any more. The files are integrated into the
> > mail repo now. I guess you're not using latest git master?
> >
> > Regards,
> > Simon
> >
> > >
> > >Should I invest effort into making some refactoring here? First of all
> > >I'd probably try to run unit tests without requiring lwip_contrib.
> > >
> > >Thanks,
> > >Mate
> >
> > _______________________________________________
> > lwip-devel mailing list
> >
>
> _______________________________________________
> lwip-devel mailing list
>

_______________________________________________
lwip-devel mailing list
_______________________________________________
lwip-devel mailing list


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel