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

Refactoring Bootstrapper to introduce better coverage and testing #1603

Merged
merged 29 commits into from Apr 17, 2016

Conversation

Projects
None yet
3 participants
@Stift
Contributor

Stift commented Apr 12, 2016

I have completely refactored the bootstrapper. The initial problem why I started this is just that this simple tool got rather complex during the last year. In the beginning it was just ~80 lines, but more and more features were integrated which introduce also bigger complexity. Therefore, I refactored it to have some test in place and also removed some bugs which I discovered btw. I think this gives some benefit to all users of paket, as everyone checks in the bootstrapper and rely on its correct functionality.

The following items are tested from now on:

  • parameter evaluation
  • download strategies (github, nuget, cached)
  • download strategy resolution
  • bootstrapping behaviour

I also improved the CachedDownload strategy which before behaved like a bootstrapper on its own and now is only a strategy, and rely that the bootstrapper will do his job.

After refactoring we should have a test coverage of ~90%, before there was nearly none. The only downside is, that my refactoring increased the size of the bootstrapper by ~5kB, which is excusable if I look on the plus side.

Besides that the following bugs were removed

  • When you call paket.bootstrapper.exe prerelease and after that paket.bootstrapper.exe, the second call does not use the latest version
  • minor issue with selecting versions in nuget strategy when prereleases were not ignored effectively

The following features were also added.

  • non evaluated command line arguments are printed on console
  • with the parameter --help a help page is now displayed with short explanation of parameters

I'm sorry for the bigger PR, but for me it seemed like the best way to succeed.

Stift added some commits Mar 31, 2016

Merge commit '0a917114f993b3e3b9917a208c768273b42ee87f' into bootstra…
…pUpdateError

# Conflicts:
#	paket.dependencies
#	paket.lock
@forki

This comment has been minimized.

Show comment
Hide comment
Member

forki commented Apr 13, 2016

clap

Show outdated Hide outdated src/Paket.Bootstrapper/DownloadStrategies/GitHubDownloadStrategy.cs
var url = String.Format(Constants.PaketExeDownloadUrlTemplate, latestVersion);
ConsoleImpl.WriteDebug("Starting download from {0}", url);
//using (var httpResponseStream = WebRequestProxy.GetResponseStream(url))

This comment has been minimized.

@forki

forki Apr 13, 2016

Member

could you please remove commented code. Hurts my OCD ;-)

@forki

forki Apr 13, 2016

Member

could you please remove commented code. Hurts my OCD ;-)

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Apr 13, 2016

Contributor

Yeah. Sorry for that. Didn't want to hurt you.

Contributor

Stift commented Apr 13, 2016

Yeah. Sorry for that. Didn't want to hurt you.

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Apr 13, 2016

Contributor

@Stift great huge job! I wish we could write it in F#, in FP style, but we cannot.

Contributor

vasily-kirichenko commented Apr 13, 2016

@Stift great huge job! I wish we could write it in F#, in FP style, but we cannot.

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Apr 13, 2016

Contributor

@vasily-kirichenko Thx.
I also would like to do that instead of doing this csharpiness stuff, but the Fsharp.Core still blocks us from doing so.

Contributor

Stift commented Apr 13, 2016

@vasily-kirichenko Thx.
I also would like to do that instead of doing this csharpiness stuff, but the Fsharp.Core still blocks us from doing so.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Apr 13, 2016

Member

I think it's fine for now, but it would make a cool case study to refactor this further to get rid of mocks and interfaces in C#.
But on the other hand I'm not sure if that brings us anything. This the bootstapper and I'm super happy that someone takes time to make that thing work well.

Member

forki commented Apr 13, 2016

I think it's fine for now, but it would make a cool case study to refactor this further to get rid of mocks and interfaces in C#.
But on the other hand I'm not sure if that brings us anything. This the bootstapper and I'm super happy that someone takes time to make that thing work well.

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Apr 13, 2016

Contributor

You can get rid of interfaces and mocks, but then you use delegates all over the place and pass them around. At least, the code is still readable. As said before, I just wanted to have tests for now and increase the coverage on that tool.
I'm absolutely happy if someone wants to join and make this cool case study (with me). Now that we have testing in place this should be even easier.

Contributor

Stift commented Apr 13, 2016

You can get rid of interfaces and mocks, but then you use delegates all over the place and pass them around. At least, the code is still readable. As said before, I just wanted to have tests for now and increase the coverage on that tool.
I'm absolutely happy if someone wants to join and make this cool case study (with me). Now that we have testing in place this should be even easier.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Apr 13, 2016

Member

yes primary goal is to make that thingy useable and get everything under test. well done!

Member

forki commented Apr 13, 2016

yes primary goal is to make that thingy useable and get everything under test. well done!

@forki forki merged commit dff687a into fsprojects:master Apr 17, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Stift Stift deleted the Stift:bootstrapperRefactoring branch Apr 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment