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

Clearing 1st pane on reset #1847

Merged
merged 5 commits into from May 22, 2014
Merged

Clearing 1st pane on reset #1847

merged 5 commits into from May 22, 2014

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 20, 2014

The 1st commit is unrelated, but quite important.

The 2nd sets up XCode's unit tests (not the Sen Testing framework - I couldn't figure out how to actually launch QS with it) and the 3rd fixes the unit tests I wrote

pjrobertson added 3 commits May 20, 2014
Add tests for checking clearing the 1st pane search string and objects under certain scenarios
@skurfer
Copy link
Member

skurfer commented May 20, 2014

Nice! I’ll look it over soon.

@pjrobertson
Copy link
Member Author

pjrobertson commented May 20, 2014

Hmmm... @HenningJ - if you have a minute, would you be able to take a look at the new unit testing stuff I've done? I was worried that it might not work with Jenkins, since it requires actually launching Quicksilver, and maybe opening the interface

(Required for Jenkins to build)
@pjrobertson
Copy link
Member Author

pjrobertson commented May 20, 2014

OK, the first problem was just that the QSTest wasn't linked against AppKit. I've done that now, and it seems Jenkins is just stalling :(

@pjrobertson
Copy link
Member Author

pjrobertson commented May 21, 2014

Hmmm... Jenkins Progress:

Started 10 hr ago
Build has been executing for 10 hr

Not what we really want to see right @HenningJ - I hope I haven't broken anything :o

@skurfer
Copy link
Member

skurfer commented May 21, 2014

I don’t see any problems with functionality, but can we clean up the broken test references?

screen shot 2014-05-20 at 11 07 39 pm

@HenningJ
Copy link
Contributor

HenningJ commented May 22, 2014

I'll try to take a look at it tonight.

But starting QS and opening the interface? That doesn't sound like a unit
test. Isn't there a better way to test this? Maybe extract the stuff you
want to test into its own method that doesn't depend on the GUI, test that
method, and then call it from the GUI? Just a thought.

On Wed, May 21, 2014 at 5:09 AM, Rob McBroom notifications@github.comwrote:

I don’t see any problems with functionality, but can we clean up the
broken test references?

[image: screen shot 2014-05-20 at 11 07 39 pm]https://cloud.githubusercontent.com/assets/154676/3035756/3d0124ec-e095-11e3-837b-f16fee253f60.png


Reply to this email directly or view it on GitHubhttps://github.com//pull/1847#issuecomment-43707999
.

@pjrobertson
Copy link
Member Author

pjrobertson commented May 22, 2014

OK @skurfer - targets removed. Not sure why that happened - I blame it on a buggy Xcode

@HenningJ - I'm not too sure if opening the interface (which I do in the unit test) is strictly necessary. If I remove that, there isn't actually anything GUI-ey going on, I make all the calls from code (e.g. I simulate a key down with an NSEvent)

@HenningJ
Copy link
Contributor

HenningJ commented May 22, 2014

Well...sounds a little better, but not much. Simulating key events still
sounds, like there is a lot of stuff going on in the background. The theory
of unit testing says that unit test should test the smallst possible unit
of code. And key event handling doesn't sound like it's a small unit of
code. ;-)

On Thu, May 22, 2014 at 5:43 PM, Patrick Robertson <notifications@github.com

wrote:

OK @skurfer https://github.com/skurfer - targets removed. Not sure why
that happened - I blame it on a buggy Xcode

@HenningJ https://github.com/HenningJ - I'm not too sure if opening the
interface (which I do in the unit test) is strictly necessary. If I remove
that, there isn't actually anything GUI-ey going on, I make all the calls
from code (e.g. I simulate a key down with an NSEvent)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1847#issuecomment-43905754
.

skurfer added a commit that referenced this pull request May 22, 2014
@skurfer skurfer merged commit 69010e4 into master May 22, 2014
@skurfer skurfer deleted the clearing branch May 22, 2014
@pjrobertson
Copy link
Member Author

pjrobertson commented May 22, 2014

Cool. I guess now Henning won't be able to test the Jenkins build (and all subsequent PR builds will fail), but this branch could always be restored.

Any light on the problem @HenningJ ?

@HenningJ
Copy link
Contributor

HenningJ commented May 22, 2014

I just got home, so I didn't have a chance to look at anything yet.
Will do so now

@skurfer
Copy link
Member

skurfer commented May 22, 2014

I guess now Henning won't be able to test the Jenkins build (and all subsequent PR builds will fail)

Oh, I didn’t think about that. Sorry. But that assumes we’re right about what’s hanging the test builds to begin with.

@HenningJ
Copy link
Contributor

HenningJ commented May 22, 2014

The last few lines for the Jenkins build log that that is indeed the problem:

...
  run-test Quicksilver Tests.xctest (macosx10.9, application-test, GC OFF)
/Users/Shared/Jenkins/xctool/xctool.sh: line 51:  3619 Terminated: 15          "$XCTOOL_DIR"/build/$REVISION/Products/Release/bin/xctool "$@"
Build was aborted
Aborted by Admin
...

So, it hangs while it tries to run the "Quicksilver Tests" target. :-(
I'm pretty sure it doesn't work because Jenkins doesn't run as a normal user, but as a user without graphical interface. So it can't do the "start the GUI" part.
I remember that being a problem for the Jenkins/iOS combination, because the iOS guys need to run the iOS simulator for their tests. I think there was some kind of workaround, but I don't quite remember.

But now that this PR is merged, it triggered a build of the master branch. And of course that hangs as well.

So, @pjrobertson could you remove/disable the offending test target and push that directly to master, at least until I figured out how to fix it?

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

3 participants