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

Add properties to IRuntimeFrameworkService and IRuntime required by GUI #249

Closed
wants to merge 1 commit into from

Conversation

ChrisMaddock
Copy link
Member

@ChrisMaddock ChrisMaddock commented Jul 1, 2017

These are the changes required for the GUI to break it's dependency on the RuntimeFramework class in the engine, and use the IRuntimeFrameworkService interface instead. This is part of #173, and has been split out of #229. It would be good to get this into 3.7 if possible, so the GUI can update, and remain on a full-released version of the engine.

The changes hopefully expose everything required here: https://github.com/CharliePoole/nunit-gui/blob/d840d1eb0c308f56391b18ce8e5f6c848e1258fa/src/nunit-gui/Model/TestModel.cs#L113 (Thanks @mikkelbu for the reference!)

While doing this, I discovered IAvailableFrameworks, which was created in nunit/nunit#1292 for use by the GUI. It doesn't appear to have ever been put into use. I decided to obsolete the existing interface for two reasons:

  1. I wanted to expose a read-only collection, as runners should not be able to add to the 'available frameworks'.
  2. I felt this belonged on IRuntimeFrameworkService, rather than as a separate interface. IServiceLocation.GetService<IAvailableFrameworks> seems like unusual syntax.

I've obsoleted it in case any other runners are using it, but I imagine it's unlikely. I thought it was worth fixing the above issues before we start using the interface - but I can revert that change if people disagree.

Note there's a couple of additional properties on IRuntimeFramework which aren't required by the GUI, but are for other aspects of #229.

@ChrisMaddock
Copy link
Member Author

The related GUI issue is at https://github.com/CharliePoole/nunit-gui/issues/209

@ChrisMaddock
Copy link
Member Author

ChrisMaddock commented Jul 1, 2017

Mono 4.2.4 keeps timing out on NuGet restore - I've ran it 3/4 times now. Mono latest has built fine.

Maybe something's down - will try it again later.

@CharliePoole
Copy link
Collaborator

Because the GUI is not a production project (I take pre-1.0 very very seriously) it has a lot of adhoc fixes and has used it's own build of the engine at some times. I wonder if something slipped into the engine as a result of an error of mine. For the same reason, however, you should be careful about adding something to the engine because it seems needed in the GUI. This is actually one of the reasons I wanted to get the GUI separated even more from NUnit.

@CharliePoole
Copy link
Collaborator

To really understand this, I will have to spend more time looking at both the engine code and the gui code. But my concern is that you are exposing more stuff to runners than we might want to expose. I'll try to find some time to say something more intelligent about that in the next day or two.

@ChrisMaddock
Copy link
Member Author

Thanks @CharliePoole. There's no urgency at all on this one - so long as you're not concerned about the engine version the GUI will be using.

But my concern is that you are exposing more stuff to runners than we might want to expose.

Interestingly, everything here I'd already added to the interface as part of my work moving stuff from RuntimeFramework to RuntimeFrameworkService. There may even be stuff that the GUI doesn't currently use - but I thought it was stuff that seemed sensible to expose.

I'm not 100% happy with the RuntimeFramework implementation, but I'm hoping to work on this (whole thing - the refactor) with a view to causing minimal changes, rather than a wholescale refactor. Will leave this with you - it can wait until after your trip!

@rprouse
Copy link
Member

rprouse commented Jul 4, 2017

@ChrisMaddock you've put this in the 3.7 milestone. I've let 3.7 slide with all the governance changes, but would like to get it out this week. Any objections moving it out?

@ChrisMaddock
Copy link
Member Author

Not if @CharliePoole is happy to use a CI build of the engine for the GUI. I only wanted to get it in the release for that. 😄

@CharliePoole
Copy link
Collaborator

Not a problem.

@CharliePoole
Copy link
Collaborator

At least, not a problem for me. If it's changing any API that could be used by other runners, then we need to be careful.

@ChrisMaddock ChrisMaddock removed this from the 3.7 milestone Jul 9, 2017
@CharliePoole
Copy link
Collaborator

@ChrisMaddock Now that I'm back from vacation, I have had a look at the details of what you did here and I see several problems.

  1. The way that we make a service public is by adding it to the API assembly. Services that are only defined in the engine are not public simply because no runner can access them. The Gui currently has a direct reference to the engine, but that's a temporary fix in order to allow it to use RuntimeFramework.

  2. I added IAvailableFrameworks to the engine API in order to make it public for use by the GUI or any other runner that needed to get a list of available frameworks. The IRuntimeFramework interface was added for the same reason and has the minimum info needed by the GUI. Because changes to the API assembly are supposed to be additive only, I didn't want to add too much information. I'm not sure why you think the syntax for getting the interface is unusual... it's the standard syntax we use to get any service.

  3. IRuntimeFrameworkService is an internal engine service, defined in the engine itself rather than the api. It can't be made public because it contains methods that are not intended to be used by anything outside the engine. The service needs to implement two interfaces because it has two different clients: runners that need to get a list of available frameworks and internal runners that need to figure out which framework to use.

  4. The gui didn't use the new interface right away, because it was not yet released. It didn't use it later on because I haven't been able to work seriously on it for almost a year, because of some of our other issues. The new issue you created (nunit/nunit-gui#209) will help keep it in the forefront of my mind. I'll see about getting it into the 0.6 release.

@ChrisMaddock
Copy link
Member Author

Thanks for taking a look Charlie. I'm away the next few days - and will need to look back at the code to work out what was going on here. Will get back to this one. 🙂

@ChrisMaddock
Copy link
Member Author

The GUI has been able to get the frameworks with the current behaviour, so there's no need to make any changes here anymore I don't think. Closing

@ChrisMaddock ChrisMaddock deleted the issue-173b branch January 20, 2018 15:03
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