Remove all deprecated functions

[keathley]

We need to update Wallaby's internal test suite and remove the deprecated functions.

[padde]

Hey @keathley I saw all the deprecations while working on the JS dialogs. I already thought about making a PR to rewrite the tests with the new API. Could you use some help here?

What about tests that test the deprecated functions themselves, should they stay or be removed altogether?

[PragTob]

What we already wanna remove them all in 0.17? That's cool, wasn't sure what your deprecation policy would be :D

I'd be super glad to remove them. Lots of them are not covered and get the test coverage down but of course writing tests for them doesn't make much sense :)

[keathley]

@padde Any help here would be great! Some of the old tests should be updated to use the new api. But a lot of them can just be deleted. For instance, all the tests like, "fill_in/3 finds elements by id" and "fill_in/3 finds elements by label text" can be deleted.

At some point I'm going to wrap the entire external api with property tests so we don't have to write all of these permutations by hand any more.

@PragTob Yup! Lets get rid of them.

[padde]

@keathley sounds great, I'd be glad to help. Just to be clear: do you want to keep the deprecated methods themselves in Browser or should they be removed as well? If they're kept, should there be at least one test for each deprecated method left in the test suite?

For the deprecation policy, SemVer might give some guidance here (if you should decide to stick with this). It generally forbids backwards incompatible changes in minor releases, but:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

[keathley]

If its easier you can just remove it all. But if you just want to work on tests that fine.

It was an interesting decision to deprecate things before we hit 1.0 but I knew that there were enough people using it and the api was a big enough change that I wanted to provide the deprecations as a way to help people migrate. Now that we have a good direction for the api I'm fine with removing the old functionality. Plus it gets us closer to a 1.0 release 😉

[PragTob]

Very pragmatic decision, I might get onto removing some of them during the weekend (I'd try to do it in smaller chunks)

[padde]

Hey @PragTob didn't want to steal this from you, please go ahead! I think it makes more sense to remove deprecated functions along with their tests and also replace them in other tests in one go. The compiler will basically tell you exactly what to do once you've removed the function, and we'd probably trip over each other's feet all the time when merging.

[PragTob]

@padde we can also work together and coordinate, I'll see what I get to this weekend because next week I probably don't have that much time :) I'm just sometimes super excited by these "stupid" tasks because all the warning messages annoy the hell out of me :D

[PragTob]

On another note, I doubt I'll get to any more than #181 at least this weekend so feel free to work away ( @padde )

[PragTob]

As a working thing, I'd say if whoever starts working on one of this just post here the name of the function you started working on removing all the deprecations (like I dunno, find or whatever). That way we can synchronize, save for find itself there also shouldn't be too much overlap :)

[PragTob]

Removing choose now

[graeme-defty]

Work on this seems to have slowed down a lot. Frustrated by trying to look through browser.ex with all the deprecated functions in place, I deleted them all. No tests failed as a result, but that is because someone thoughtfully removed the failing tests ;-)

The removal of the tests has been pushed, but not the removal of the actual deprecated functions.

Would my changes (i.e. the deletions) make a useful pull request? Does someone already have this and waiting for a suitable moment to release it? Have I misunderstood all the foregoing comments completely?

Graeme

PS A bit off-topic, but I have raised a couple of "issues" which were really just calls for help. Is there a better place for such discussions?

[PragTob]

Hi there @graeme-defty - the tests aren't removed yet. I suspect you ran mix test but since a recent test you need to run mix test.all for which lots of tests should be failing then, at least I get lots of deprecation warnings.

Work has slowed down a bit here, true. I'm busy with non wallaby stuff and a lot of work also went into the selenium stuff - I might get back to some function removals though.

I don't see any issues from you. As we don't have a mailing list, this is a good place to raise issues imo. Other places can be #wallaby on slack or elixirforum.

[graeme-defty]

Yes, exactly what I was doing. I have plenty of errors now, thanks.

Happy to lend a hand with this, if it helps.

g

On 7 May 2017 at 19:17, Tobias Pfeiffer notifications@github.com wrote:

Hi there @graeme-defty https://github.com/graeme-defty - the tests aren't removed yet. I suspect you ran mix test but since a recent test you need to run mix test.all for which lots of tests should be failing then, at least I get lots of deprecation warnings.

Work has slowed down a bit here, true. I'm busy with non wallaby stuff and a lot of work also went into the selenium stuff - I might get back to some function removals though.

I don't see any issues from you. As we don't have a mailing list, this is a good place to raise issues imo. Other places can be #wallaby on slack or elixirforum.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keathley/wallaby/issues/177#issuecomment-299702585, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK80KwrfNIoG4CsQEKtqyN_lyiltcKEks5r3bZogaJpZM4MvwL1 .

[graeme-defty]

OK, so I looked at the very first deprecated function in browser.ex. This turned out to be check/n. My concern here is that it seems that removing check and uncheck removes functionality from the API.

As it stands, I can set (check), unset (uncheck) or toggle (click) the value of an option. Removing check and uncheck is a reduction in capability since now I can only toggle the value. Of course I can test the value first, but that puts the onus on the tester to do extra work that the framework can easily handle.

Not only that, this API is the tester's main way to specify testing operations, and should be as rich as possible, meaning as close as possible to how a user views his interactions with the browser screen. This makes tests much more readable.

My apologies if I have missed the point here. Also, please bear in mind that I am new to the project, so if this has been discussed ad nauseam elsewhere please point me to the conversation and I will try to alleviate my ignorance.

Thanks,

graeme

[PragTob]

Hi there,

your master seems to be out of date. check was already removed in 894b72d0cb5

I can see your concern about the removed functionality, this could best be discussed in a separate issue that you could open and then also tag @keathley in there as he first defined those functions as deprecated and I'm sure he had an idea about it :)

I'm not sure about them. You are right though, click only is a real replacement for toggle. One might argue that the tester should know from the test setup which fields are checked and which aren't so that it doesn't make a difference and in the end what the user does is clicking.

But as I said, that's probably easier done in a separate issue and easy to restore :)

Tobi

[graeme-defty]

Hi Tobi

Thanks for the advice and guidance.

I will (a) make sure I am working off the latest master and then (b) open another issue making the case for another review of the interface. I am still pretty new to Wallaby, so I hope I am not asking folks to go over old ground, but I feel this is important as this API is a big part of how the user views the framework.

Thanks again.

graeme

On 7 May 2017 at 23:09, Tobias Pfeiffer notifications@github.com wrote:

Hi there,

your master seems to be out of date. check was already removed in 894b72d https://github.com/keathley/wallaby/commit/894b72d0cb50de14e7b238f5b1735662aa50709a

I can see your concern about the removed functionality, this could best be discussed in a separate issue that you could open and then also tag @keathley https://github.com/keathley in there as he first defined those functions as deprecated and I'm sure he had an idea about it :)

I'm not sure about them. You are right though, click only is a real replacement for toggle. One might argue that the tester should know from the test setup which fields are checked and which aren't so that it doesn't make a difference and in the end what the user does is clicking.

But as I said, that's probably easier done in a separate issue and easy to restore :)

Tobi

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/keathley/wallaby/issues/177#issuecomment-299716196, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK80F8xevniKjUl4zNs5DSeGbw-CwwFks5r3eylgaJpZM4MvwL1 .

[PragTob]

@graeme-defty don't worry about making us go over old ground, "worst" case there is already an issue/comment where the thinking is explained and we'll just link to it :) Thanks for contributing to wallaby!

[keathley]

@graeme-defty Thanks for the comments! I agree that we're loosing some functionality by removing check and uncheck. But the bigger idea behind these deprecations is to separate querying from actions. I'll use click_link and click_button as an example for a moment and then talk about why I feel like removing check and uncheck is a good idea.

The only reason to have click_link and click_button is to make it easier to specify the element that you actually want to click. We can achieve the same semantics by simply saying, click(link("Some Link")) or click(button("Some Button")). A great side benefit of this approach is that the end user no longer has to learn Wallaby's api rules. They get a few simple actions and can compose them any way that they want. This moves the declarative power out of the action api and into the query api. This movement is a huge benefit to myself as a maintainer because its easier to build stateless transformations. I also get a benefit as a user of the api because I can specify exactly what I want to interact with as a query.

With check and uncheck I agree that we're loosing some semantic power from the "Actions API". The goal is to move that power to the Query api. check and uncheck is very similar to click_link and click_button in that you're trying to specify the state of the element as part of the action. In other words, check and uncheck are trying to specify query logic as part of the click action.

My goal is to add a new option to the query api that would allow you to specify the "selected" state of the element. This would be equivalent to an uncheck action: click(checkbox("My checkbox", selected: true)).

Hopefully all of that makes sense. I'm happy to discuss more of these design decisions if you have thoughts on them. I appreciate you thinking and asking about this stuff :+1:

[PragTob]

That makes sense to me at least, thanks for taking the time to explain @keathley ! :)

[graeme-defty]

@keathley OK – let me try to outline my thinking in the most compelling way (experience tells me that I quite probably have only one shot at this, so it had better be good! :-) )

Apologies in advance for the length of this post, but I want to make sure I express myself clearly and there are a number of facets I want to explore.

First let me say thank you for taking time to listen. I am not being (intentionally) difficult. I am taking an interest because I think Wallaby has a great future. The expressiveness allowed by Elixir is perfect for this kind of framework, and I really like the direction in which Wallaby is heading.

Second, with respect to your email, and in particular the second paragraph (The only reason … ) rest assured that I absolutely do get the introduction of Query as the encapsulation of the act of locating an element. And an important benefit of this is that it reduces the number of unit tests dramatically even in those functions which remain. No longer do we have click_element_found_by_css, click_element_found_by_name, click_element_ … and endless other combinations. The act of finding the element is separated from the action taken on it, and the two can be tested independently. In fact I have some suggestions on further encapsulation of Query that you may like to consider, but I will save that for another discussion. I want to get to the main course.

The next 2 paragraphs in your mail highlight where we are looking at this from different perspectives.

When it comes to click_link, click_button, et al I could not agree with you more. All of these elements are simple (i.e. single-level) structures that have no state. You click them and that’s it. It really makes no sense to have multiple actions here.

The checkbox, conversely DOES have state. And a simple click reverses that state. This is very very different from a link or a button. The action you propose click(checkbox("My checkbox", checked: true)). is different from uncheck in one important regard. The query in the former will fail if the checkbox is not checked. And as a tester, there will be many cases where I really don’t care what value it has initially, I simply want to ensure that after my action, it will have the desired state.

Now Tobi raised this point in his response - a tester has the ability to know exactly the state of his test environment, and can in principle know how a checkbox is initially set, but that may depend on a number of factors, and this is an added complexity that I do not believe the tester should be forced to handle.

Let me give a concrete example.

Say I have a screen which adds a new user to my system and there is a group of personal interests/hobbies represented by checkboxes, some of which are set on by default. Now if I want to add a user for some test. I need the user to have a specific set of interests, but that is not the focus of the test. In such a situation I do not want to concern myself here with what the default values are. (I have another test to check the defaults are correct.) I just want to add a user. At some future time the default values may change. Then my test will fail and will need to be changed for what seems to me to be no good reason. The defaults may even come from a config file. Or perhaps the defaults may be generated based on other information we have already entered about this user. Granted this is all still under my control, but it leads to brittle tests.

Stepping back a moment, there is a certain elegance in the ‘everything is a find or a click’ approach. A minimal number of basic concepts which can be composed to build anything the tester needs. However, although this can be done, that does not necessarily mean that we should do it. A good analogy here is the Elixir language itself, which has a very small number of basic constructs and a very, very cool macro facility. So cool in fact that many of the basic language constructs we take for granted are actually macros. (It is a while since I read about this, so forgive me if I have mis-remembered, but I believe that ‘if’ and ‘case’ are examples.) Now Jose could have given us just the basics and the macros and left us to construct the rest of Elixir ourselves, but I do not think we would have thanked him for it. A minimal set of orthogonal concepts is great for under the covers, but the bones are better if we join them up and put a bit of meat on them.

So it is a judgement call. We have to pitch somewhere on the scale of “bare minimum of composable features and you can build the rest yourself” to “here are so many concepts and ways to do everything that it will make your head spin”.

Usually, the truth lies somewhere in the middle. But where?

For me, a good target for the language of describing tests (i.e. our API) is the language we would use if we were describing to a user over the phone how to perform the tests manually. I could of course say to my remote user “look at this checkbox and if it is checked then click it” but I am much more likely to simply say “make sure this checkbox is unchecked”. The telephone call analogy appeals to me because it is almost exactly the situation we are in with respect to the communication we have with someone who is reading our tests down the track. It is much easier for him if we speak to him (i.e. write our tests) in the language we would use in talking to a user sitting in front of our browser interface.

I totally agree with your point about minimising the learning curve for the tester (your point about having to learn Wallaby rules). We have the same goal here. But if we go the minimalist approach he has to learn our set of minimal constructs and the rules for composing them. The best way to minimise his learning curve in IMHO is to use take our API closer to his language - i.e. the language we would use in the telephone call situation.

Well, apologies again for the length of this missive, and again thanks for listening to my views. I hope I have done my job well enough, but I hope that it has at least been useful.

g

[graeme-defty]

OR ... Maybe the way to look at it is that some DOM elements have a value and others do not.

Things without value would be links and buttons. You can click them and that's it.

Things with value include check boxes, radio button groups, drop-down lists and text boxes (any others?). The set_value function could be retained to set the value of these elements. Depending on the type of the element set_value operates appropriately, setting a checkbox to true or false, selecting a particular radio button from a group, selecting one option from a drop-down list or performing a send_keys operation for a text box. That gives us Query, click and set_value as the primitive concepts. A fairly short list, and while not quite as obvious as check and uncheck, I think still fairly clear and readable.

What do you think?

[keathley]

@graeme-defty No worries about all of the text. I really do appreciate you taking the time to thinking about this stuff. :heart:

I think keeping set_value around makes a lot of sense. Its not as clear as check or uncheck but it allows users to compose their own actions in a more meaningful way. I need to do some more thinking about this stuff. We are going to have to make exceptions for other actions like attach_file. I just need to think about where we draw the line.

[graeme-defty]

Sounds good. Sorry I did not think of the that suggestion first. I could have saved myself a lot of typing and you a lot of reading!

[graeme-defty]

I just posted a pull request for removing deprecated attr/2. (Stayed with a baby one :-) )

I am looking now at what needs to be done to set_value.

[graeme-defty]

I have created a PR for the extension to set_value so it now works with check boxes, and incidentally, radio buttons and drop-down list options.

[keathley]

All deprecated functions were removed with v0.17. :tada: