QuickCheck experiment #3

seliopou · 2013-10-10T15:29:34Z

This pull requests includes QuickCheck tests for the OpenFlow0x01.Wildcards module. During the course of implementing the tests, I found that the nw_src and nw_dst have type int but those should be uint, if OCaml had such a type.

Some questions:

Is it worth going doing roudtrip testing with QuickCheck for all the serializable types in this package?
A subordinate question: Doing so requires exposing currently private functions (i.e., marshal and parse). Is this acceptable?
Where should QuickCheck "instances" reside? Separate from the module providing the datatype requires that the datatype no longer be abstract.

For my perspective, the answer to these questions are: yes, yes, and it should go in the module where the datatype is defined.

parse . marshal == id

reitblatt · 2013-10-10T16:11:45Z

On Thu, Oct 10, 2013 at 11:29 AM, Spiros Eliopoulos <
notifications@github.com> wrote:

This pull requests includes QuickCheck tests for the
OpenFlow0x01.Wildcards module. During the course of implementing the tests,
I found that the nw_src and nw_dst have type int but those should be unit,
if OCaml had such a type.

Do you mean uint? OCaml int types are quite a pain, I personally think we
shouldn't be using "int" at all in that module. Those should probably be
Int32.

Some questions:

Is it worth going doing roudtrip testing with QuickCheck for all the
serializable types in this package?

Yes! But that's a bit of a task. I would actually prioritize OpenFlow0x04
since that's what we're using for the PLDI development. The first 7 cases
of OpenFlow0x04.Messages.t are probably what we really should cover.

A subordinate question: Doing so requires exposing currently private
functions (i.e., marshal and parse). Is this acceptable?

Where should QuickCheck "instances" reside? Separate from the module
providing the datatype requires that the datatype no longer be abstract.

For my perspective, the answer to these questions are: yes, yes, and it
should go in the module where the datatype is defined.

I agree. I don't know what unit test Best Practices (TM) are, but this
seems reasonable to me. So would we then have a test() method in each
module that gets called from Test.ml?

You can merge this Pull Request by running

git pull https://github.com/frenetic-lang/ocaml-openflow quickcheck

Or view, comment on, or merge it at:

#3
Commit Summary

qc: add quickcheck as a dependency

qc: expose 0x01.Wildcards parse and marshal

qc: Implement 0x01.Wildcards roundtrip tests

qc: note OF0x01.Wildcards fields that are unsigned

File Changes

M INSTALL.txthttps://github.com/QuickCheck experiment #3/files#diff-0(3)

M _oasishttps://github.com/QuickCheck experiment #3/files#diff-1(2)

M _tagshttps://github.com/QuickCheck experiment #3/files#diff-2(4)

M lib/OpenFlow0x01.mlhttps://github.com/QuickCheck experiment #3/files#diff-3(4)

M lib/OpenFlow0x01.mlihttps://github.com/QuickCheck experiment #3/files#diff-4(24)

M setup.mlhttps://github.com/QuickCheck experiment #3/files#diff-5(10)

M test/Test.mlhttps://github.com/QuickCheck experiment #3/files#diff-6(57)

Patch Links:

https://github.com/frenetic-lang/ocaml-openflow/pull/3.patch

https://github.com/frenetic-lang/ocaml-openflow/pull/3.diff

seliopou · 2013-10-10T16:34:19Z

Yep that was a typo. I meant uint.

Are the OpenFlow0x04 datatypes completely independent from OpenFlow0x01? Arbitrary instances do tend to build on top of each other, but if they're independent I'll start looking at OpenFlow0x04 sooner rather than later.

I envision having just the arbitrary instance in the module where the datatype was defined, exposing parse and marshal, and then putting it all together in Test.ml. I'll make those changes before this is merged in.

reitblatt · 2013-10-10T16:44:13Z

On Thu, Oct 10, 2013 at 12:34 PM, Spiros Eliopoulos <
notifications@github.com> wrote:

Yep that was a typo. I meant uint.

Are the OpenFlow0x04 datatypes completely independent from OpenFlow0x01?
Arbitrary instances do tend to build on top of each other, but if they're
independent I'll start looking at OpenFlow0x04 sooner rather than later.

Not quite sure what you mean by arbitrary instances, but OpenFlow0x04
diverged a fair bit from OpenFlow0x01 for most of the message types. I'd
say they're fairly independent, but Marco/Arjun have been in that code much
more recently than I have.

I envision having just the arbitrary instance in the module where the
datatype was defined, exposing parse and marshal, and then putting it all
together in Test.ml. I'll make those changes before this is merged in.

Sweet! With unit tests, we might be able to pass for a real software
project!

jnfoster · 2013-10-10T16:44:53Z

Small correction: I suggest not prioritizing 0x04 over 0x01 (Mark's paper
is not the only project using this).

-N

On Thu, Oct 10, 2013 at 12:34 PM, Spiros Eliopoulos <
notifications@github.com> wrote:

Yep that was a typo. I meant uint.

Are the OpenFlow0x04 datatypes completely independent from OpenFlow0x01?
Arbitrary instances do tend to build on top of each other, but if they're
independent I'll start looking at OpenFlow0x04 sooner rather than later.

I envision having just the arbitrary instance in the module where the
datatype was defined, exposing parse and marshal, and then putting it all
together in Test.ml. I'll make those changes before this is merged in.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-26069325
.

reitblatt · 2013-10-10T16:50:05Z

On Thu, Oct 10, 2013 at 12:44 PM, Nate Foster notifications@github.comwrote:

Small correction: I suggest not prioritizing 0x04 over 0x01 (Mark's paper
is not the only project using this).

True, but OpenFlow0x01 has been fairly stable for a while now, right? While
OpenFlow0x04 was just hacked up. So, testing should be directed at the more
complicated/less tested code, no?

As per the discussion on #3, arbitrary instances will reside in the module where the datatype it's testing resides. Because of this, the quickcheck library is now a dependency for the openflow library, as well as the test suite.

arjunguha · 2013-10-10T19:26:49Z

_oasis

@@ -20,7 +20,8 @@ Library openflow
    cstruct,
    cstruct.syntax,
    lwt.syntax,
-    packet
+    packet,
+    quickcheck


I recommend making this an optional dependency, by creating an openflow.quickcheck sub-library. See how it is done here:

https://github.com/frenetic-lang/netkat/blob/master/_oasis

arjunguha · 2013-10-10T19:30:08Z

I recommend putting *_arbitrary into a separate library. Keep the structure flat. I'd like to also flatten the structure of the OpenFlow parsers by using OCaml 4.01

jnfoster · 2013-10-10T20:45:05Z

0x01 is unfortunately not all that stable. But the code is a bit more
structured and polished. So not to micromanage, but I think it makes sense
to vet / harden it first, then build on that experience to impose the same
level of rigor on the 0x04 code. And we need 0x01 for the abstraction layer
anyway.

-N

On Thu, Oct 10, 2013 at 12:50 PM, Mark Reitblatt
notifications@github.comwrote:

On Thu, Oct 10, 2013 at 12:44 PM, Nate Foster <notifications@github.com

wrote:

Small correction: I suggest not prioritizing 0x04 over 0x01 (Mark's paper
is not the only project using this).

True, but OpenFlow0x01 has been fairly stable for a while now, right? While
OpenFlow0x04 was just hacked up. So, testing should be directed at the more
complicated/less tested code, no?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-26070570
.

arjunguha · 2013-10-10T21:37:55Z

Nate, Mark, you need to get on our hangout with marco and spiros. It is caleld FLAT TIRE.

This reverts commit db9ae56.

This reverts commit 53f84d1.

Use the OpenFlow0x01_Arbitrary signature to build all the components of a roundtrip test. Then it will be straightforward to run a test using the values defined in the module that conforms to that signature: module M : OpenFlow0x01_Arbitrary quickCheck M.arbitrary M.to_string M.parse M.marshal

seliopou · 2013-10-11T01:20:35Z

@arjunguha This should be good to go.

LGTM

Blocking issues for merge

seliopou added 4 commits October 9, 2013 23:25

qc: add quickcheck as a dependency

3e2972c

qc: expose 0x01.Wildcards parse and marshal

4447a20

qc: Implement 0x01.Wildcards roundtrip tests

425d952

parse . marshal == id

qc: note OF0x01.Wildcards fields that are unsigned

ba83849

qc: Move OF0x01.Wildcards arbitrary to module

53f84d1

As per the discussion on #3, arbitrary instances will reside in the module where the datatype it's testing resides. Because of this, the quickcheck library is now a dependency for the openflow library, as well as the test suite.

ghost assigned arjunguha Oct 10, 2013

qc: remove unused module alias

db9ae56

arjunguha reviewed Oct 10, 2013
View reviewed changes

seliopou added 3 commits October 10, 2013 18:02

Revert "qc: remove unused module alias"

ccb4cce

This reverts commit db9ae56.

Revert "qc: Move OF0x01.Wildcards arbitrary to module"

99c8ffe

This reverts commit 53f84d1.

arjunguha added a commit that referenced this pull request Oct 11, 2013

Merge pull request #3 from frenetic-lang/quickcheck

ee1e5d0

LGTM

arjunguha merged commit ee1e5d0 into master Oct 11, 2013

mcanini pushed a commit that referenced this pull request Jul 4, 2014

Merge pull request #3 from fugitifduck/multipartReply

d49a714

Blocking issues for merge

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QuickCheck experiment #3

QuickCheck experiment #3

seliopou commented Oct 10, 2013

reitblatt commented Oct 10, 2013

seliopou commented Oct 10, 2013

reitblatt commented Oct 10, 2013

jnfoster commented Oct 10, 2013

reitblatt commented Oct 10, 2013

arjunguha Oct 10, 2013

arjunguha commented Oct 10, 2013

jnfoster commented Oct 10, 2013

arjunguha commented Oct 10, 2013

seliopou commented Oct 11, 2013

QuickCheck experiment #3

QuickCheck experiment #3

Conversation

seliopou commented Oct 10, 2013

reitblatt commented Oct 10, 2013

seliopou commented Oct 10, 2013

reitblatt commented Oct 10, 2013

jnfoster commented Oct 10, 2013

reitblatt commented Oct 10, 2013

arjunguha Oct 10, 2013

Choose a reason for hiding this comment

arjunguha commented Oct 10, 2013

jnfoster commented Oct 10, 2013

arjunguha commented Oct 10, 2013

seliopou commented Oct 11, 2013