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

Adding an alert when not finding the Test class #4

Closed
wants to merge 1 commit into from

Conversation

kenji21
Copy link
Contributor

@kenji21 kenji21 commented Jun 10, 2015

Delayed menu injection because the Find -> "Jump to Test File" was not appearing
Add some logs in exception that prevents Xcode from crashing

Added an alert to copy test class name if not found in project :

screen shot 2015-06-10 at 19 47 47

Added an alert to copy test class name if not found in project
@kenji21
Copy link
Contributor Author

kenji21 commented Jun 10, 2015

Also take some investigation over "Navigate menu issue", but no luck.

I've used NSMenuDidChangeItemNotification to log these changes... it seems that it changes dynamically on project/file opening

@marksands
Copy link
Owner

Very cool, thanks! I'll take a look at it when I get a chance. I need to get the unit test suite back up and running... also on my todo list!

@marksands
Copy link
Owner

After reviewing this I'm going to have to reject it. I have some feedback that will help us both in the future.

There are 2 features and 1 bug fix in this pull request in a single commit. I cannot stress enough to group context sensitive changes into separate commits. I see at least 3 here:

  1. A bug fix for the menu option not appearing
  2. Display an alert view with the option to copy the test class when it's not found
  3. Add NSLog if an exception is thrown when creating the menu item

If you would like to open a pull request for the bug, I'd be happy to merge it. Unfortunately, I'm not interested in adding an NSLog to the plugin--I don't think NSLogging is particularly useful in this situation.

For item 2, I have other ideas. There are currently scenarios that exist where I have a test class, but it's not found in the plugin. For example, assume a class ABCDataMigrator.m exists, but the tests for it are so broad I've separated them into multiple files such as ABCDataMigratorModel1Tests.m and ABCDataMigratorModel2Tests.m. This is a real scenario that tools such as AppCode provide an alert with a selection of possible files to jump between. I'd like a feature that behaves similar to that, and when it cannot find the test file then you can optionally associate one, or more, with the source file.

@marksands marksands closed this Jun 14, 2015
@kenji21
Copy link
Contributor Author

kenji21 commented Jun 15, 2015

Ok, so we need to add more buttons to the alert like :

  • "Cancel"
  • "Open ABCDataMigratorModel1Tests.m/swift"
  • "Open ABCDataMigratorModel2Tests.m/swift"
  • "Copy ABCDataMigratorModel1Tests.m/swift"

Maybe I can implement this and send another PR ?
A better feature would be to look in "opened file history", and re-open the first file we could open we find in the history.

Ok for NSLogs, I like to got them with a "dev/debug" version because we can't debug the plugins (last time I tested, got a crash when Xcode spawns another Xcode).

But unit tests are here for that :-)

@marksands
Copy link
Owner

Ok, so we need to add more buttons to the alert like :

As for the "alert", I'm thinking more along the lines of a dropdown selector; see the images on FuzzyAutocomplete and Code Pilot for what I'm thinking--I'm not interested in a plain alert view with buttons.

A better feature would be to look in "opened file history", and re-open the first file we could open we find in the history.

I'm not sure what you mean by that statement. I don't think there's any advantage to searching for recently opened files. Xcode knows which files are test files, and it's not terribly difficult to match up a test file with it's main counterpart at least the majority of the time.

Ok for NSLogs, I like to got them with a "dev/debug" version because we can't debug the plugins (last time I tested, got a crash when Xcode spawns another Xcode).

If the plugin isn't working then it's a clear indication that you need to launch Aviator within Xcode to start debugging. You're free to do what you want in development, such as scatter NSLogs around or enable Xcode's All Exceptions breakpoint. I just ask that you please not commit these into the repository. I'm very picky about code 😊

I appreciate the interest you've taken!

@kenji21
Copy link
Contributor Author

kenji21 commented Jun 16, 2015

Hmm, seems there is three cases :

  • No tests files found
  • Only one test file matches
  • More than one tests files matches

For the first, the "NSAlert" looks good to me, the second is already implemented. And the third need a "custom UI", I tried a simple NSMenu :

screen shot 2015-06-16 at 23 06 38

Forgot "to clipboard" in the third choice. Esc allow to leave this menu, but this menu is only "mouse", no up/down arrows to select the choice (maybe it can be tweaked).

What needs to be changed is to have "referenceCollectionForSourceCodeDocument:" return an array of TFFFileReferenceCollection.

@marksands
Copy link
Owner

Hmm, seems there is three cases :

No tests files found
Only one test file matches
More than one tests files matches
For the first, the "NSAlert" looks good to me, the second is already implemented. And the third need a "custom UI", I tried a simple NSMenu :

I definitely do not want an NSAlert telling me there are no tests. We can do better. We should have the ability to directly open the XCTest File Template and allow the user to create the test file.

Forgot "to clipboard" in the third choice. Esc allow to leave this menu, but this menu is only "mouse", no up/down arrows to select the choice (maybe it can be tweaked).

I'm not sure NSMenu is the proper UI component for this behavior; I would think an NSView that holds a list of selectable elements would be prime. See my previous comment with examples and pictures.

What needs to be changed is to have "referenceCollectionForSourceCodeDocument:" return an array of TFFFileReferenceCollection.

I'll give it some thought. That may be appropriate.

@kenji21
Copy link
Contributor Author

kenji21 commented Jun 17, 2015

I definitely do not want an NSAlert telling me there are no tests. We can do better. We should have the ability to directly open the XCTest File Template and allow the user to create the test file.

Sure, but the issue I had is that we can't know in which group to put the newly created file (aside the "under test file" like you do, or in the "BundleTests groups". Then the second issue is to "simulate user input to create the new file", so the easy way for me is to have the test class name copied, and then the user can easily right click into the group he wants the to be (and maybe create this group just before).

I'm not sure NSMenu is the proper UI component for this behavior; I would think an NSView that holds a list of selectable elements would be prime. See my previous comment with examples and pictures.

Sure it is not the best, but it can be added very quickly, and then see if the "not so often" usage of this menu still requires a better UI.

@marksands
Copy link
Owner

Sure, but the issue I had is that we can't know in which group to put the newly created file (aside the "under test file" like you do, or in the "BundleTests groups".

I see what you mean, and it's a fair point. I think with a first pass it can default new test files to be added to the Tests group, since that probably aligns with most developers--I understand my style is unique. If that were the case, it would be great in the future to add a preferences window to be able to specify where default tests were created, if alerts should be shown, etc.

Also, if there is an alert view, I like how AppCode presents it. It's not in your face and it is automatically hidden after a about a second of delay.

Then the second issue is to "simulate user input to create the new file", so the easy way for me is to have the test class name copied, and then the user can easily right click into the group he wants the to be (and maybe create this group just before).

Not sure why you think we'd have to simulate user input to create a file. We have the entirety of Xcode's private API at our fingertips! 😄 👍 It should be totally possible.

Also, I want to stress that this is not intended to be easy, I mean look at the dance I went through to scrap through Xcode's private headers just to get this far! 😨 But what I am saying is that nearly anything is possible.

Sure it is not the best, but it can be added very quickly, and then see if the "not so often" usage of this menu still requires a better UI.

Ok, I'm not 100% against it, it just feels like it's improperly being used.

I don't want you to feel pressured into doing this if it sounds overwhelming. I'm capable of implementing these features and would gladly do them. If you want to contribute some to this idea--please do--but just be aware that I tend to be hypercritical 😉. It may be a good idea to break these up into different issues even.

@kenji21
Copy link
Contributor Author

kenji21 commented Jun 17, 2015

Sure the better the UI is, the most it will be usable. Maybe there is two or three days of work for making this feature. The great thing is that AppCode is making Xcode better (and vice-versa). Having access to some private "APIs" is also very interesting to add such feature to Xcode. I don't know so much Mac APIs (iOS primary), but would you consider PR to a feature branch to implement these features ?

@marksands
Copy link
Owner

Sure the better the UI is, the most it will be usable.

👍

Maybe there is two or three days of work for making this feature.

I'm in no rush to get any of these features in. If it takes 3 days or 3 weeks, that's fine. The main thing I'm concerned about is code quality and a solid, functioning plugin.

The great thing is that AppCode is making Xcode better (and vice-versa).

There's more of an onus for Xcode to improve, that's for sure!

Having access to some private "APIs" is also very interesting to add such feature to Xcode.

Yes, I hope you don't think Xcode plugin architecture is documented, because it's not. It's all one giant hack, it kind of sucks. This is a good article on what goes into building Xcode plugins.

Here is a dump of the private headers. It's a little out of date, but that's what I've been using for reference.

I don't know so much Mac APIs (iOS primary), but would you consider PR to a feature branch to implement these features ?

I will open some new issues with some detail regarding what we've discussed in here. I think it's all good information. If you want to work on a feature branch and let me know, I'd be happy to provide feedback.

But as I stated, Xcode plugin development is a tricky beast. It's a lot of hacks strung together and requires reading a lot of reading of private headers and it can be painful to debug certain things in order to make them work. I really think this is a very involved task that will take many many days to get 100% complete. I also appreciate unit tests written where they make sense (when working in private Xcode API land).

If that doesn't interest you, then I'd consider accepting pull requests for the alert/menu, preferences pane, etc. Just keep me updated on what you do. If you do decide to work on some of this, I would like to give feedback along the way so that you don't end up wasting a lot of time. 😄

@marksands
Copy link
Owner

I created issues #5, #6, and #7

@kenji21
Copy link
Contributor Author

kenji21 commented Jun 18, 2015

Thanks, I've used some plugin mentionned (XcodeExplorer) to made some internals plugins in my company (that should be pushed to github, but there are more proof of concept when I wrote them, so code quality is not so high...)

I've seen the Ctrl-R button from #5 to launch tests for selected file(s) : "wahou", very interesting feature...

I'll post on issue before trying some dev on it (and I'll made a more "unit" commit this time).

Regards

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

2 participants