YahooArchive / arrow

FE Test framework designed to promote TDD
http://yahoo.github.io/arrow/arrow_intro.html
BSD 3-Clause "New" or "Revised" License
55 stars 59 forks source link

Add test config and param for overriding window size. #205

Closed ryanashcraft closed 10 years ago

ryanashcraft commented 10 years ago

This is an attempt to fix issue #196. We require different tests for different window sizes because our site has a responsive design. This change allows us to set the window size for each test, instead of the window being maximized with every test.

pranavparikh commented 10 years ago

@ryanashcraft

Thanks for the PR.

Are you expecting it to work from command like --defaultWindowSize={"width":"x","height":"y"} ? Can you provide an example for the usage and also add a test ? Thanks

ryanashcraft commented 10 years ago

@pranavparikh

I have modified the feature so that it may be used in two ways:

As a config (exposed as a command-line arg):

arrow --defaultWindowSize=1024,768 test.js

As a test param in a descriptor (from tests/functional/data/window_size_test/window_size_test.json):

"Set browser size" : {
    "group" : "func",
    "browser":"firefox",
    "params" :{
        "page": "$$config.baseUrl$$",
        "test": "test-browser-size.js",
        "windowSize": {
            "width": "512",
            "height": 512
        }
    }
}

"windowSize" param will take precedence over the global defaultWindowSize config value.

I have added two functional tests for the defaultWindowSize arrow config and the windowSize test parameter. Both tests check to see if the browser's window.innerHeight/innerWidth are set correctly.

Looking forward to your feedback.

pranavparikh commented 10 years ago

@ryanashcraft

Any reason for having "defaultWindowSize" ? Instead of having two params - defaultWindowSize and windowSize, we can have only param - "windowSize". By default, we will maximize if "windowSize" is not defined in the descriptor. Thoughts ?

ryanashcraft commented 10 years ago

@pranavparikh That is acceptable. I will update the PR accordingly.

ryanashcraft commented 10 years ago

@pranavparikh I have removed the --defaultWindowSize interface, functionality, and test.

pranavparikh commented 10 years ago

@ryanashcraft This is not working with locator controller.

{ "TestYHOOTicker": { "group": "func", "params": { "scenario": [ { "page": "$$config.baseUrl$$" }, { "controller": "locator", "params": { "value": "#txtQuotes", "text": "yhoo\n", "windowSize": { "width": "400", "height": "300" } } }, { "test": "test-quote.js", "quote": "Yahoo!Inc.(YHOO)" } ] } } }

ryanashcraft commented 10 years ago

@pranavparikh: The issue likely lies in that SeleniumDriver.prototype.navigate does not have a testParams argument. So right now it's just calling resizeWindow without an argument.

I was trying to avoid changing existing function headers. Would it be acceptable to add an optional testParams argument as the last argument for SeleniumDriver.prototype.navigate?

Thanks for the feedback.

pranavparikh commented 10 years ago

@ryanashcraft ,

We can pass "testParams" in navigate instead of just "page" and get page as "testParams.page". Since, we have "testParams" now, we can pass windowSize to resizeWindow().

-SeleniumDriver.prototype.navigate = function (page, callback) { +SeleniumDriver.prototype.navigate = function (testParams, callback) {

and do this in default.js

if (!self.testParams.test && self.testParams.page) {
pranavparikh commented 10 years ago

@ryanashcraft Do you foresee a situation where you want to have same window size for all tests ? I that's the case, we can support it by passing default size in config.js and then having descriptors override it , if needed.

ryanashcraft commented 10 years ago

@pranavparikh:

I have updated to add support for custom window sizes in locator controller. The issue was locator controller doesn't call executeTest or executeAction, so now instead locator.js explicitly calls resizeWindow. I also changed navigate as you described to take in testParams, so you can now:

            "scenario": [
              {
                "page": "$$config.baseUrl$$",
                "windowSize": {
                  "width": "400",
                  "height": "300"
                }
              },
              ...
            ]

Not sure if that's really helpful, though.

Regarding a default size – we don't have a need for it. I'm sure someone might have a use for it, but whoever does can build that in themselves.

pranavparikh commented 10 years ago

@ryanashcraft Why do we need this for locator controller ? I don't see a situation where user will need to resize a window while clicking on an element or waiting for some element. Or am I missing something ? Users can resize after loading the page as you mentioned.

ryanashcraft commented 10 years ago

@pranavparikh: I thought you wanted it to work with locator controller, per your feedback from two days ago.

pranavparikh commented 10 years ago

@ryanashcraft ,

Ah got it now. Actually, that will work for scenarios now once we pass testParams in navigate() method. So we dont need to resize at controller level. Like

"scenario": [ { "page": "$$config.baseUrl$$", "windowSize": { "width": "400", "height": "300" } }, ... ]

ryanashcraft commented 10 years ago

@pranavparikh, Ok, that makes sense. I reverted the change to location controller. The previous change I made to SeleniumDriver.navigate handles scenarios as you described.

pranavparikh commented 10 years ago

@ryanashcraft

Some changes were made as part of https://github.com/yahoo/arrow/pull/211. Can you please sync up your fork with master ? Until then, I can't take in your PR.

Thanks

ryanashcraft commented 10 years ago

@pranavparikh

I have rebased from master. Please let me know if there's anything else you need to accept the PR. I appreciate your feedback.

Thanks, Ryan

pranavparikh commented 10 years ago

@ryanashcraft Thanks for the PR.

ryanashcraft commented 10 years ago

@pranavparikh Thank you for your feedback and merging it.