formfactor.coffee now tests against real user agents #456
Conversation
|
Sorry for the replicated commits, not sure how to keep my branches clean. |
|
Can someone help me pass the Jasmine tests? The failing code is in formfactor.coffee |
|
What kind of help do you need exactly? To make the specs run manually, or to make them pass? |
|
To make them pass. I think it's probably an issue with the coffeescript I wrote. I'm new to the language. |
|
Ok let me look at what you did. |
|
I don't know too much about UserAgents, but should these 2: and really be from a tablet browser? Cause, these are the 2 cases making the build to fail. It's detected as a mobile instead of a tablet. |
|
Yes they're iPad useragents |
|
Oh yes I see the mobile in the ua. Perhaps the tests are working correctly and the actual formfactor code is buggy. |
|
Yeah most probably. |
|
Thanks for the help. I think this validates using actual useragents in the tests. |
|
A remark though, about CoffeeScript, I think it would be more readable in the tests if you would write them like this (let me know what you think of it) : desktopUserAgents =
[
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1664.3 Safari/537.36",
"Mozilla/5.0 (Macintosh; Intel Mac OS X 1083) AppleWebKit/537.36 (KHTML like Gecko) Chrome/28.0.1469.0 Safari/537.36"
]
tabletUserAgents =
[
"Mozilla/5.0 (iPad; U; CPU OS 3_2 like Mac OS X; en-us) AppleWebKit/531.21.10 (KHTML, like Gecko) Version/4.0.4 Mobile/7B334b Safari/531.21.10",
"Mozilla/5.0 (iPad; U; CPU OS 4_2_1 like Mac OS X; ja-jp) AppleWebKit/533.17.9 (KHTML, like Gecko) Version/5.0.2 Mobile/8C148 Safari/6533.18.5"
]
mobileUserAgents =
[
"Mozilla/5.0 (iPod; U; CPU iPhone OS 3_1_1 like Mac OS X; en-us) AppleWebKit/528.18 (KHTML, like Gecko) Mobile/7C145",
"Mozilla/5.0 (iPod; U; CPU iPhone OS 2_2_1 like Mac OS X; en-us) AppleWebKit/525.18.1 (KHTML, like Gecko) Version/3.1.1 Mobile/5H11a Safari/525.20"
]
describe 'finding from user agent', ->
describe 'for a mobile device', ->
for userAgent of mobileUserAgents ->
it 'should be mobile when userAgent is ' + userAgent, ->
expect(findFormFactorFromUserAgent(userAgent)).toBe 'mobile' |
|
Agreed, my for loop was backwards, I'll patch it now. |
|
Last question. Is it possible to move the userAgent arrays to separate .coffee files? I think they're going to end up quite large. |
|
And, out of curiosity, what'll be the scope of this test? How are we going to maintain it? UserAgents strings always change. |
|
I think we'll never have full coverage but the idea will be to never remove a ua string from the array and then to add strings as we discover them. My best source for ua strings so far is: I think it updates over time so maybe we can do some kind of diff every month or so and add the new ua's. I know this sounds like a horrible pain even as I say it. |
|
Fair enough. Also, for your question, yes there is a way. It's a bit hacky though, since CoffeeScript's scoping is... somewhat... "special"... You could create another file, say, userAgents.coffee window.userAgents =
[
...
]and reference it in your tests for userAgent of window.userAgents ... |
|
Ok thanks, I'll separate them out tomorrow. Also the current code in master incorrectly detects iPad's as mobile instead of tablet so I recommend merging this fix before the next release. |
|
I need some details here. So you have UASorter which takes a huge list of User-Agents and populates the coffee script lists? Where does the first list come from? I'm gonna merge your other PR and wait a little bit for this one, I'll wait for your answers before I do a review :) |
|
The unsortedUserAgent.json comes from converting http://techpatterns.com/downloads/firefox/useragentswitcher.xml to json. Each time you run the uasorter program it selects a random useragent from unsortedUserAgent.json It then prompts the user to supply the formfactor and adds the useragent to the arrays in mobileUserAgents.coffee, desktopUserAgents.coffee, tabletUserAgents.coffee based on the answer. Then it replaces the useragent in unsortedUserAgent.json with null, so that it won't ask for it again. When tests are run mobileUserAgents.coffee, desktopUserAgents.coffee and tabletUserAgents.coffee supply useragents to formfactor.coffee which tests to see if the formfactor function interprets them correctly. |
|
All other commits can wait but commit 439bc52 fixes the formfactor for ipads so it should probably be merged as soon as you're able. |
|
@Pacane I'm pretty sure I've had a syntax error in formfactor.coffee since replacing the for loops, but the tests didn't fail. I assume because they weren't being run. You might need to add a test to see that formfactor.coffee compiles. |
|
@Pacane I've added a user agent string to mobileUserAgents which should cause the tests to fail. But the tests pass. Can you check that window.desktopUserAgents etc is available to the formfactor.coffee code. |
|
@Pacane tests fail correctly now, so ignore last message. Probably should still add a test to ensure formfactor.coffee compiles. |
|
@rdwallis I'm pretty sure that there's a problem though. Usually Jasmine would make a test fail on a compile/syntax error for either CoffeeScript or Javascript. However here, I only get messages like In the UI, instead of an actual failure. (UI being the one given by mvn jasmine:bdd) |
|
Yes if you have a syntax error in the coffee file the tests always pass, so we need to test the coffee files. I'm not sure how though. |
|
Here's an issue at the jasmine project: Edit: looks like it's not the correct project though the error is very similar |
|
I think it would be the same problem if we wrote the tests in JS, right? Have you tested it? |
|
I've added zFinalTest.coffee which can test if the other tests have run. I named it with a z since tests seem to run in alphabetical order. This will catch all syntax errors except for the ones in zFinalTest.coffee since it can't test itself. |
|
If you write the tests in JS then you can catch and fail on exception so we won't have this issue. |
|
Oh wait we can just make zFinalTest.coffee into a JS file and it should solve our problems. |
|
I don't care about coffeescript though, if we can make the whole thing more robust, we can write the tests in JS. I've just picked coffee, cause I find it easier to write. Push your changes though, I'm interested in seeing how you managed to do this. |
|
Ok the codes all committed. I'm not actually sure that zFinalTests.coffee works but you can see the idea. I think converting everything to plain javascript is probably the best solution. |
|
I see what you did, but I don't see how it prevents the build from passing when there are compilation errors. |
|
Each variable checked by zFinalTests.coffee is set in one of the other coffee files. If one of the other coffee files doesn't compile then its variable will be undefined and zFinalTests.coffee will report the error making the build fail. Anyway I think converting everything to plain js is a better solution. |
|
Well if you want to rewrite, go ahead, but it works well with that fix. I like that 👍 |
|
This is subsumed by PR #458 |
No description provided.