Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gen.uri #331

Closed
wants to merge 6 commits into from
Closed

Gen.uri #331

wants to merge 6 commits into from

Conversation

ploeh
Copy link
Member

@ploeh ploeh commented Dec 4, 2016

This pull request is mostly a request for comments, and shouldn't be seen as an entirely complete body of work.

The aim is to provide building blocks for creating various forms of Uri values. This is done though various 'role' types that each have their own generators and shrinkers. This pull request introduces the following Arbitraties:

  • UriScheme
  • UriHost
  • UriPathSegment (no shrinker yet)

Using these building blocks, it's possible to create truly arbitrary Uri values:

> (sprintf "%O://%O/%s") <!> Arb.generate<UriScheme> <*> Arb.generate<UriHost> <*> (Arb.generate<UriPathSegment> |> Gen.map string |> Gen.listOf |> Gen.map (String.concat "/")) |> Gen.sample 10 10;;
val it : string list =
  ["Ez://[834f:1436:ac78:3d5f:d5a1:c388:fe50:692e]/zr-k";
   "RElM04://[e7ac:ce44:10d5:f76d:39fe:2096:b884:79e8]/h~SsGg/6YD/rj2CQIi1/c8N1B/Bt_M0Sx.lZ";
   "nntp://[4466:32a8:6d8f:5d1:96b8:2efa:bfe1:57a9]/pDvaO2Uz~n/OogZ~";
   "net.tcp://rmfd4yhaya2zdbimgmdhb39c4y48ztx3uosyptntkoi-fjd5ae60.jo/.L/W-2Hhv91G/hVNs/6/r5XCQq/cqi";
   "https://v4iy-2m0bv1smqhnhlcicg8dhb3-k0717a2w26xrv1s.versicherung/V/IW";
   "net.tcp://254.246.113.219/"; "net.tcp://bugatti/mrWwo7/SQ4mvao/dAmFyU";
   "mailto://[1ec1:9c47:ac39:c387:1b37:3842:6cbf:53e9]/YDdrjXCQI/JoCu8i/8/TyMEeX.L/eXx/3IWOo7h~N1/o7HzN/~S/ydRJXCc";
   "news://31.174.120.145/9uu7-xKw/my7h/_ThTNxHH0/LC/0F";
   "net.pipe://2w0rx1vm-eo.aeg/Mml1.g/e_0B_Pbt/4NJjbp3IA/F/fo-UWNsG"]

, or, if you want only 'web' URLs:

> sprintf "%O://%O/%s" <!> (Gen.elements ["http"; "https"]) <*> Arb.generate<UriHost> <*> (Arb.generate<UriPathSegment> |> Gen.map string |> Gen.listOf |> Gen.map (String.concat "/")) |> Gen.sample 10 10;;
val it : string list =
  ["https://76.164.217.72/2/nfYyME"; "https://ntkoc-iu9iqxrv1.xperia/";
   "http://7udja59e6.qjkh64sph3zmee5i1he.business/9RH61/JZBr-VoQ/RS1GUuI5/80Af/R7LlfO/m0XxLldW/V.q9Jw0MJ/3d9A/9W_Su59jBS/OZ";
   "https://marriott/Vldnp/dvWAi/n/Ew-P3VvJ"; "https://gs/JqMssh6H5";
   "https://azure/_M0/SQ10hRIWw/Ie0s/-dsHvo/ft7ZESs";
   "http://[bf8b:1c6:e8b4:2aef:1187:5318:3ab0:7c9e]/YnnNj_w/OEN";
   "http://gd/PLlzr5KY";
   "https://[5e0c:5d6c:563b:9507:2343:bc00:8b90:47cc]/M66x.3IWw/YT29_L2/LP9CDMrFf/HWB74A/OokcQ43u/gku~LV3/LLVW5WBPp";
   "https://ug/t3XhlQaomK/VKhw0yp80"]

Actually, these examples cheat a bit, because they only generate strings, but one can easily generate Uri values from the above generators by using Gen.map Uri or similar techniques.

This PR doesn't include support for ports, query strings, authentication data, or fragments, because I first wanted to ask whether this is even something that's interesting to include in FsCheck.

If so, it's also my intent to introduce a built-in Uri Arbitrary that combines all of the above Arbitraties, as well as a built-in WebUrl Arbitrary that generates only HTTP-based URLs (for use with testing of e.g. RESTful services).

This particular type addresses the concern of generating scheme values for
Uri values.
This particular type addresses the concern of generating the host part of
Uri values.
A URI host can be either an IP address or a host name. When defining a
shrinker, it'll be important to determine if the original value was an IP
address or a host name.
@kurtschelfthout
Copy link
Member

This is looking really good. I think in terms of which arbitrary types are good for inclusion in FsCheck: as many as possible! :) The only time I'd draw a line is if it would incur an additional dependency; in those cases I have previously offered to start a separate project under the FsCheck organization and that offer still stands.

As far as this PR is concerned, it's looking great.

One suggestion, it looks like this is developing in a sort of sub-API to generate Uris. I wonder if we can make the connection between the different Uri-related instances clearer by grouping them together in the API somehow - perhaps on a separate class, so you'd see them as Arb.Default.Uri.UriPathSegment for example.

|> System.String
|> UriScheme }
// While these well-known scheme are available in the full BCL as
// Uri.UriSchemeFile, Uri.UriSchemeFtp, etceterat, they aren't
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: etceteratetcetera

@ploeh
Copy link
Member Author

ploeh commented Dec 16, 2016

I like the idea of having a 'sub-API' like Arb.Default.Uri.UriPathSegment, but how will that work in practice? I don't see anything that looks like that on Arb.Default currently, so I'm wondering, if adding a new nested class like Arb.Default.Uri would work.

I can't say that I fully understand the way FsCheck picks up Arbitraries, but would these Arbitraries automatically be included if they belong to a new type?

@kurtschelfthout
Copy link
Member

They wouldn't be automatically picked up, but you can just add the nested class here once and for all: https://github.com/fscheck/FsCheck/blob/master/src/FsCheck/Arbitrary.fs#L165

@ploeh
Copy link
Member Author

ploeh commented Dec 17, 2016

OK, thank you; I'll try that.

This PR is falling behind master, so I'm going to close it as is, do some work on it, rebase it on master, and open a new PR when it's ready for another review.

@ploeh ploeh closed this Dec 17, 2016
@ploeh ploeh deleted the gen-uri branch December 17, 2016 10:44
@ploeh
Copy link
Member Author

ploeh commented Dec 17, 2016

How about calling it Arb.Uri instead of Arb.Default.Uri?

@kurtschelfthout
Copy link
Member

No objections. Please consider how that interacts with "utility" methods on Arb like Arb.convert etc. (I understand there is no technical interaction as they are different methods/types, I mean from an API usability/clarity perspective)

@ploeh
Copy link
Member Author

ploeh commented Dec 19, 2016

That may be an argument for Arb.Default.Uri... I think I'll try that first, then.

Thank you.

@ploeh
Copy link
Member Author

ploeh commented Dec 28, 2016

How would you even implement something like Arb.Default.Uri? Arb.Default is a static class (not a module), so in order to support that nested syntax, Arb.Default.Uri would have to be a nested class. AFAICT, though, F# doesn't support nested classes.

Any ideas?

@kurtschelfthout
Copy link
Member

I was thinking it would just be a property Uri that returns an instance, but now that you mention it then I don't think it would work with the Arbitrary default registration mechanism without some changes (or you'd have to do some extra work to wrap the static members (for Arbitrary registration) into and instance (to return from the Uri property). Not sure if it's worth it.

Could also turn the Default class into a module, but not sure if that's worth it either.

@ploeh
Copy link
Member Author

ploeh commented Dec 31, 2016

Could also turn the Default class into a module, but not sure if that's worth it either.

Is that possible? AFAICT, the reason that it's a class in the first place is to work around F#'s restriction against circular references. That's the main reason that so far, it's resisted all my attempts at refactoring it, as mentioned in another thread.

@kurtschelfthout
Copy link
Member

Is it something rec doesn't solve? As I said, not sure it's worth it in any case.

@ploeh
Copy link
Member Author

ploeh commented Jan 4, 2017

I'm not sure I follow...

@kurtschelfthout
Copy link
Member

Is that possible? AFAICT, the reason that it's a class in the first place is to work around F#'s restriction against circular references.

I guess I don't understand where the circular references come into play here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants