Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc - mainly fixing unit tests. Fixes #1509 #1508

Merged
merged 8 commits into from Jun 13, 2013
Merged

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jun 2, 2013

This mainly fixes the unit tests (except one, which should be fixed by another pull request - UTIs), plus adds 'Open' as an alternate to the AS 'Run' action.

I'm contemplating doing the same for the shell 'Run' action, but I can't remember where the action is now :/

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 5, 2013

Good to see this. I was hoping to see the special exceptions for mailto: and javascript: go away, but I guess that would require a return to using NSURL and checking scheme. What was the problem with that again? 😃

One small problem with this (and another test we need to add): If you enter anything that's a valid TLD suffix by itself, it's seen as a URL. Try "com" or "uk" or "it".

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 6, 2013

but I guess that would require a return to using NSURL and checking scheme. What was the problem with that again?
hehehe, why did you have to bring that up eh? ;-)
Looking through the commits - 7eb3045 seems to point to where I took the NSURL method out. I give a few reasons there :)

I'll add a check for TLDs only and fix that. Cheers

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 6, 2013

OK, fixed.

Did I tell you how easy it is to fix things like this with unit tests? Nice work ;-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 6, 2013

P.S. we still need special cases for matilo: and javascript: since as I see it, they're not 'real' matching schemes (they don't have :// in them) so they're harder to sniff generically.
We should probably compile a list (like we did for the TLDs) of valid schemes (that don't use ://)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 6, 2013

If you're interested, that last commit adds XN--J1AMH as a TLD. Not sure what it is ;-)
I just ran the check blindly and checked the diff afterwards

@tiennou
Copy link
Member

@tiennou tiennou commented Jun 6, 2013

Does the IDN TLDs are recognized with this ? Like say, http://test.укр (that's xn--j1amh). Else, you'll have the immense pleasure of having to write a Punycode to UTF8 decoder ;-).

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 6, 2013

You can use either http://test.укр or http://test.xn--j1amh

That's the interesting thing about these TLDs. You're right, QS won't be able to pick them up though... ;-)

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 6, 2013

I started trying different things in the UI, then remembered that we already have a more reliable way to test and this has already passed. Lovely. It's going to take some getting used to. 😃

I'm good with merging this if we aren't going to worry about .укр.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Jun 6, 2013

Did I tell you how easy it is to fix things like this with unit tests? Nice work ;-)

I started trying different things in the UI, then remembered that we already have a more reliable way to test and this has already passed. Lovely. It's going to take some getting used to. 😃

I tried to get you hooked on unit testing a while ago. But I guess I didn't have quite the right hook.
Good to see it's working for you now. :-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 7, 2013

I tried to get you hooked on unit testing a while ago. But I guess I didn't have quite the right hook.
Good to see it's working for you now. :-)

Yeah, we need to thank you as well Henning. I think you initially set up the tests right?
I've started playing with RoR (@tiennou - Ruby QS coming soon? :D ) and was using unit tests there, so I saw how good they are for not just fixing things, but actual writing the code in the first place.

On 6 Meh 2013, at 23:23, Henning Jungkurth notifications@github.com wrote:

I tried to get you hooked on unit testing a while ago. But I guess I didn't have quite the right hook.
Good to see it's working for you now. :-)

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 7, 2013

Who's going to create a ton of tests for QSCatalogEntry and QSLibrarian? 😉

@tiennou
Copy link
Member

@tiennou tiennou commented Jun 9, 2013

Ruby QS coming soon?

Given the "sad" state of MacRuby, not yet... There are times where I rewrite my Ruby code in ObjC (when time is available) but it's a daunting task. I'm more like, writing notes about how to architect this correctly from the ground-up, by accounting for the new things in OS X since, what 10.1 (how old is that code anyway ;-)). Using CoreData, yes/no ? Using FSEvents everywhere, yes/no ? Using UTIs, yes/no ? Lather, rinse, repeat.

BTW, since you're now a Ruby developer, take a look at rspec for one of the most impressive things Ruby tests can do ;-).

Edit: I meant cucumber, but it's usually used in cunjunction with rspec.

Who's going to create a ton of tests for QSCatalogEntry and QSLibrarian?

Heh. Multi-threaded tests.

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 10, 2013

Ran into a problem this morning and a bisect led me to 491daed.

Try selecting multiple file objects with the comma trick. 😟

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 10, 2013

Uuuugh yuck. Good find.

The problem is here

Turns out if an array is saved for a type in an object, then objectForType: should return nil and not the array. Maybe this is wrong, and file objects should be changed to manage this properly, but I'm not sure :/

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 10, 2013

OK, yes. If you look at QSObject_FileHandling.m:L479, then you'll see that objectForType: expects either nil or a single path. Whereas with the change that's causing the problem, and array is being returned.

The root of the problem is that QSCollections are being treated (interchangeably) with QSObjects. Maybe @tiennou will come out of his hole to fix this one right now? ;-)

When arrays are stored for types under an object's data, they should not be returned in `objectForType:`
Instead, `arrayForType:` should be used to return the array
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Jun 10, 2013

OK, I've fixed that last one and added a couple more unit tests, with better explanations (other than just // p_j_r doesn't know why :P)

Seems like arrayForType: should be used if you actually want an array. Fragile? Yes...

@skurfer
Copy link
Member

@skurfer skurfer commented Jun 10, 2013

Yeah. See #861. I'll try this with the changes later.

skurfer added a commit that referenced this issue Jun 13, 2013
Misc - mainly fixing unit tests. Fixes #1509
@skurfer skurfer merged commit 272e6e9 into quicksilver:master Jun 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants