combinators, spy-xhr, reader interface

[cryogenian]

• Extracted combinators from slamdata. But, changed their names to mimic Selenium module and webdriver method names.
• Added xhr-spy. (Now we can in example wait until next PUT request will be replied)
• switched from Eff to Aff in setFileDetector, it's not really necessary, but all other functions in that module are Affs
• checked by cryogenian/slamdata#245d2e6c6de65b897a1a102724a9d32cd953b8f7

[jonsterling]

Nice! I'm glad to get a lot of this stuff moved into the webdriver library.

Regarding the change from Eff to Aff in setFileDetector, I wonder if this is the right thing to do—it does make the API a bit easier to use in practice, but the types also document the capabilities that are required by a piece of code, and this change makes it seem as though setFileDetector is asynchronous, when it is not.

I don't care that much, so feel free to keep it in Aff if nobody else brings it up; just thought I'd register my thoughts here.

By the way, is cryogenian/slamdata#245d2e6c6de65b897a1a102724a9d32cd953b8f7 the correct sha? That looks like something from July.

[jonsterling]

--> "If spying has not been set, this will raise an error"

[cryogenian]

fixed

[cryogenian]

It's last commit of wip/selenium-extracted branch

[jonsterling]

@cryogenian Ah, thanks! I had naïvely pasted that path into GitHub, forgetting that their URLs don't quite work in the way I expected.

[jdegoes]

I don't care that much, so feel free to keep it in Aff if nobody else brings it up; just thought I'd register my thoughts here.

My 2 cents: Since almost everything in this lib is Aff, and since Aff can represent sync computations as well, my personal prefer would be to keep it in Aff. Otherwise it just forces the user to liftAff in random places in the API.

[jonsterling]

OK; 👍 from me.

By the way, with regard to things like spying on requests and waiting until some request is responded to, I wonder if this is the kind of thing that we actually want to be doing in tests. It results in our tests making intensional (implementation-sensitive) observations about our app, whereas for predictability & non-fragility, purely extensional observations seem better. Here are a few examples:

1. We might change a button action to result in a POST instead of a PUT; the behavior observable to the user remains the same, but our test may break.
2. Completion of a request may not necessarily indicate that our test is free to proceed to the next step; in general, this kind of condition must be UI driven, I think.

Perhaps I've misunderstood the purpose/use-case of spying on requests though...

[jdegoes]

Yeah, it feels a bit like mocking, which I hate. Tests should care about what happens not how. That said, I don't know what the use cases are, so hard to say for sure.

[jonsterling]

@jdegoes Yeah...

By the way, I think it's fine to have this tool in the library, since someone may find it useful. But we may want to take a look at our own tests (separately from these PRs) and see if it is the best way to accomplish what we want.

One thing I worry about is that it might cause subtle timing issues, resulting in spurious test failures. For instance, if we wait until a request completes and then proceed to another test, we have made the tacit assumption that a UI transition that makes the next test applicable will have occurred simultaneously with the completion of that request, which is not the case. If the test proceeds in the window between request completion and UI transition (and this will be non-deterministic in general), we'll have a very frustrating thing to debug 😬

EDIT fixed typos

[cryogenian]

We test not only UI, but all browser related things as well. Checking that PUT http://foo.bar.baz have been sent and replied isn't less informational then, in example, showing button or changing src of img.

In fact all our tests are implementation sensitive. That's why there is huge config 😄

[jdegoes]

Well, the point is, if the PUT is supposed to result in some UI change, we can test for that UI change directly and ignore the PUT. If it isn't supposed to result in a UI change, well, the UI probably needs fixing. 😄

Anyway, is this ready for merging???

[jonsterling]

@cryogenian I guess I don't believe that we should be testing things like that request at all... Additionally, I think we have to draw the line somewhere (and it will be arbitrary, I know) for what things are considered "user observable" and "mere implementation details". My perspective is that it is reasonable to draw the line at "stuff in the HTML"; so, the stuff we have in the config, namely CSS classes, etc., should be considered observable, whereas what particular network request we send should not be considered observable.

[jonsterling]

If @cryogenian is ready for this to be merged, I give my 👍

[cryogenian]

I think it can be merged.
@jonsterling One option is to test mongodb content and that it's changed.

[jonsterling]

@cryogenian I don't think that we should be testing that mongodb content has changed in the frontend tests at all...

[cryogenian]

@jonsterling Heh, I've the very similar discussions couple times before 😄