<elopio> vila: please take a look at http://bazaar.launchpad.net/~elopio/selenium-simple-test/print_elements/revision/375
<vila> elopio: hehe, even shorter ! So you don't need the mock ones anymore right ? <evil grin>
<elopio> vila: I disagree with you there :)
<vila> elopio: I'm all ears
<elopio> vila: this tests are nice to have, but they don't test a unit of code, they are retesting things that are already tested in selenium, they depend on firefox to run, they are slower, and the set up for the test is done on an external file (index.html), thus are not as readable as the other ones.
<vila> elopio: ha, then don't write them this way
<elopio> vila: now I'm all ears ;)
<elopio> vila: the way to solve all those problems is with a unit test and mock. I agree there are some code paths not tested on the original
<elopio> for example, _get_text
<vila> elopio: your implementation could be more testable but it would mean to not rely as much on actions.py then to be able to test it with a simple hmtl snippet
<vila> elopio: didn't you implemented some helpers to get there without involving firefox ?
<elopio> vila: no, firefox is always needed. I tried with htmlunit, but it sucks.
<elopio> we could do something with phantomjs.
<elopio> that would make it faster than firefox, but still slower than the mocked unit test.
<vila> the mocked one is faster because it does less, but you don't get a clear idea of what 'less' means by just reading the tests, you need to know the implementation to write them which is... bad
<elopio> vila: I wrote them before implementing them.
<vila> elopio: knowing how the implementation will work as opposed to knowing only the API
<vila> elopio: you test actions._element_to_string but mock element.get_attribute
<vila> elopio: how do you know they are related ?
<elopio> knowing what the function does, not how it is implemented.
<elopio> it is a really low level function, so it's implementation is straight-forward.
<elopio> but the implementation could change, what the tests describe is what it does. When there's no id attribute, then get the text.
<elopio> lets say we have a way to test an html snipped without a huge delay
<elopio> then
<elopio> element = mock.Mock()
<elopio> element.get_attribute.return_value = None
<elopio> element.text = 'Test text'
<elopio> would be the same as
<elopio> element = '<p>Test text</p>'
<elopio> the latter is shorter and clearer, I agree.
<vila> elopio: no, the later is shor.. see ?
<vila> elopio: moreover, even after reading the mock tests thrice, I still don't get what they do, if I really want to understand, I need to look at the implementation
<vila> elopio: and I still don't understand why there is a if in test_element_with_id.mock.get_attribute but a single call...
<elopio> vila: maybe I can make them clearer, for example making a function called get_element_without_id, and that creates and return the mock.
<vila> elopio: you'll end up duplicating the implementation which is even worse ;)
<elopio> no, let me try.
<elopio> vila: here: http://bazaar.launchpad.net/~elopio/selenium-simple-test/print_elements/revision/376#src/sst/tests/test_actions.py
<elopio> once we have a beautifulsoup selenium driver, or something like that, we can change the _get_element functions to their shorter versions.
<elopio> but for the moment, we either test a lot more of things, like the test I added before, or use mocks.
<vila> elopio: test_element_with_id is testing that  actions._element_to_string is calling element.get_attribute
<vila> elopio: but it doesn't test element.get_attribute
<elopio> vila: no. It's testing that when you call an element with an id, the id is returned.
<elopio> and I don't have to test get_attribute because it is tested in selenium.
<elopio> we chose selenium in part because it has a nice test suite, so we don't have to duplicate all the tests that are there.
<vila> elopio: no it isn't "testing that when you call an element with an id, the id is returned", that's the trap, it does *because* that's what you put in the mock
<vila> elopio: keep the test and change the implementation and it will still succeed
<vila> elopio: not for *any* kind of change, but for some
<elopio> vila: for me, it would be just as doing <a id=test</a>
<elopio> it will succeed as long as get_attribute('id') is called.
<elopio> that's why the if is set on the test setup.
<vila> "as long" is the key
<vila> that's part of the implementation
<elopio> vila: it's the same. With the html string it will succeed as long as get_attribute('id) is called.
<vila> elopio: but with the string you're not faking the implementation
<vila> elopio: which is why I dislike mocked test without a double using the real code to cover
<elopio> vila: you are either testing the whole selenium+browser combination, or find a way to fake them to make them a lot faster.
<vila> elopio: debugging bogus tests is way longer than any gain in running them
<elopio> we already have tests for selenium+browser, so it would be a waste of resources to do that again.
<elopio> and we don't have a full-featured test double, but we can mock parts of that easily.
<vila> elopio: ECONTEXT
<vila> what are those those already existing tests you're talking about ?
<elopio> vila: they are in our SST'´s acceptance suite, and in selenium.
<vila> elopio: do they cover the change you're proposing ?
<elopio> vila: the change I'm proposing has nothing to do with selenium+browser combination. It only affects webelement.
<vila> meh, webelement is part of selenium right ?
<elopio> vila: right.
<vila> so it *has* to do with selenium
<elopio> vila: we know that webelement is properly tested. I know that whenever I call get_attribute('something'), that attribute is going to be returned.
<elopio> why would I have to test it again?
<vila> elopio: let me return the question: why are you testing it again ?
<vila> elopio: why don't you just acquire that element once and do all the tests you want on it ?
<elopio> vila: that's what I'm doing. I'm just faking the element acquisition, because I know it's tested, and that's not part of my change.
<vila> elopio: no, you're modifying that element, that's what I'm objecting about
<elopio> I'm not modifying any element. On the test setup, I'm returning an element that has the id attribute set to 'Test id'.
<elopio> If I do it with an html string, that would be more readable. I fully agree.
<elopio> but the element I return would be the same, in the duck sense.
<elopio> let me try to formulate a question.
<elopio> lets say I have the quick and test double selenium driver that doesn't requires firefox.
<elopio> lets say I implement it using beautiful soup and a string. So I extend the selenium's get function to receive an HTML string instead of a page URL. And I implement all the get_attribute and find methods in that  test double.
<elopio> So, on the test I would be able to change _get_element_with_id to something that opens <a id='test'></a> on my test doube, and I would be able to return fake_browser.get_attribute('id')
<elopio> In that case, I'm not testing selenium's get_attribute. I'm using the method from the double.
<elopio> I just found a more readable way to set up my test.
<elopio> hum, that wasn't a question :)
<elopio> vila: would you agree with that kind of test double?
<-- elopio (<email address hidden>) has quit (http://quassel-irc.org - Chat comfortably. Anywhere.)
--> elopio (<email address hidden>) has joined #u1-reviews
*** Mode #u1-reviews + by irc.canonical.com
* Channel #u1-reviews created on Wed Aug 29 01:32:01 2012
<vila> elopio: same issue
<vila> elopio: either you test at the proper level (which may require selenium test internals knowledge to write the tests) or you don't
<vila> elopio: if you don't and want to use some shortcut to validate some assumptions, fair enough. As long as you still have a test that use the real code :)
<vila> elopio: in other words, each time you take a shortcut, you open a hole, I don't like holes because bugs love them
<elopio> vila: I can see our disagreement there :D
<elopio> let me try to expalin my possition. However, I know I'm not going to convince you.
<vila> elopio: note that I'm *currently* fighting a lack of tests that would have allowed me to refactor easily and that I'm trying to diagnose a bug that seems unrelated to my current work
<vila> elopio: why is that so ?
<vila> elopio: because the test is too high level and involved far too much code
<vila> elopio: there are two ways this can be addressed: I can write mock tests to isolate the issue and be done or I can write more tests to make sure the lower levels are sane and that I use them without fear or I can debug and fix the issue without adding tests
<elopio> I am testing all the possible paths for the code I wrote. That's the real code for this MP.
<elopio> I am not introducing holes, because all the selenium code paths are tested there, and by just running our test suite + the selenium's test suite, we will know that this MP is fully covered *
<elopio> * with the _get_text exception. I'll probably add a test for that today.
<elopio> So instead of filling holes, what you propose is duplicating tests. In my opinion.
<vila> yeah, that makes 3, find the bug ;)
<vila> elopio: have you looked at the selenium code and tests or is it just a guess ?
<elopio> vila: I'm sure your bug doesn't have to do with lower selenium levels, like get_attribute.
<elopio> vila: I've looked at selenium tests, and contributed with a couple. So I had to read them to understand how to make new ones.
<vila> elopio: if they have tests for webelements, why don't you get their test helpers to create the webelements you need and get rid of both issues (firefox and mock) ?
<elopio> and what I think is the right solution for your problem is "write more tests to make sure the lower levels are sane", but I would mock selenium to make sure I'm testing SST lower levels, instead of selenium lower levels :)
<elopio> vila: their tests are slow. They open the browser and do stuff. In that case, it wouldn't make sense to mock selenium, as they are testing selenium.
<elopio> well, it might make sense for some kinds of tests.
<vila> elopio: you mean they don't have unit tests for webelement ?
<elopio> vila: I don't know if they have unit tests for webelements. The once I studied where higher level than unit. They had an html file, open a browser, call the selenium actions, just like our acceptance tests.
<elopio> vila: here: http://code.google.com/p/selenium/source/browse/java/client/test/org/openqa/selenium/ElementAttributeTest.java
<elopio> I think, there are no code paths untested, therefore, no holes.
<vila> elopio: I'm not arguing about that, I'm arguing about the fact that you make assumptions about selenium implementation and your don't cover them with tests that don't make those assumptions
<mfoord> all unit tests now pass
<vila> elopio: selenium implementation may change, your mock tests won't notice, the real ones will
<-> salgado is now known as salgado-lunch
<elopio> vila: I consider those assumptions to be covered in this selenium suite.
<elopio> and there you are talking about an integration test, that should be completely different. Instead of testing _element_to_string, I should test get_attribute. I agree we  are missing them.
<elopio> but to wrap-up, should I land the MP with the unit tests and acceptance tests? or should I remove the unit ones?
<vila> elopio: feel free to land, I never objected about having *both* ;)
<elopio> vila: I know, this was just because I was missing disagreeing with you :) It was getting boring.
<vila> elopio: my remark was just that the day the unit ones break... you will remove them ;-p But since it's unlikely that selenium change that specific part, you can sleep peacefully ;0D
<vila> lol
<elopio> vila: I can work on the integration tests, if you consider them a priority.
<vila> nope
<elopio> I think they are really nice to have, and a whole new world that we are missing on all the projects.
<vila> yeah, but your last url reminded me that selenium is written in java and we have better things to do that play with java tests ;)