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.

Sign up for GitHub

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

Jump to bottom

Prepared working solution of Postgres that can be used with integration tests #1

Merged
merged 14 commits into from
Feb 1, 2018
Merged

Prepared working solution of Postgres that can be used with integration tests #1

merged 14 commits into from
Feb 1, 2018

Conversation

zabrowarnyrafal
Copy link
Member

Prepared working solution for Windows and Linux, compatible with net standard 2.0.

@marcinbudny
Copy link
Contributor

I'm unable to comment on the files in the PR since it's too big. Here are some gathered comments:

  • You need to include PostgreSQL license copy into each postgres copy
  • The postgres binaries now total at over 400mb. Is it even feasible to put this into nuget package?
  • you need to account for package installation location in .net core, which is the user's home folder
  • current implementation of disposable pattern is wrong. When called from finalizer, you should not access other reference objects. See https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern I think given nature of the library (integration testing tool) you should just go for simple implementation of 'Dispose()' method
  • code in constructor of PostgresRunner silences exceptions. How will user learn if / what went wrong?
    • Let's move the code out of the constructor.
  • allow customization of database name in returned connection string
  • the State is unnecessary, right?
  • unique hash would be more unique if just left as a GUID. but isn't port number enough instead of hash? just make sure to clear the dir on startup if it was left on the disk during previous crashed execution

@zabrowarnyrafal
Copy link
Member Author

zabrowarnyrafal commented Jan 29, 2018 •
edited
Loading

Ad1: #2 (this will be done when code be ready for publishing)
Ad2: indeed; osx binaries cause oversize of tools; for now only win64 and lin64 solution will be prepared and osx solution as more effective managing Pg binaries will be postponed to one of next releases after nuget package will be published.
Ad3: need to test and fix eventual issue before 0.1.0 release; IMO not need it to be part of this PR
Ad4: this have to be fixed; will be part of this PR
Ad5: diagnostic need to be rethinked; further enhencement - maybe 0.3.0 release
Ad6: not necessary part of PR; this must be definitely done before 0.2.0 release
Ad7: usage need to be rethinked; not need to be change till 0.1.0 release; IMO can be done after 0.3.0 release
Ad8: GUID (or UUID) usage can cause path to temp folder to be too long; 6-characters hash should be enough; port number could be also used; clearing of directory in test setup procedure must be implemented before 0.1.0 release but not necessary part of this PR

To summarize: ad2, ad4 need to be fixed as a part of this PR. Rest can be done as a different PR.

@marcinbudny
Copy link
Contributor

5 and 6 have easy fixes and significant value.
8: Currently paths are generated under C:\Users\[user]\AppData\Local\Temp, so you won't exceed MAX_PATH

@marcinbudny
Copy link
Contributor

One more comment: use an options object instead of single parameters to configure call to PostgresRunner.Start - this way it will be easier to change things in the future and keep backwards compatibility.

Rafal Zabrowarny added 4 commits January 30, 2018 23:06
Removed OSX binaries; OSX as OS Platform won't be supported till issu…
24a1bda 
…e of it binaries size will be resolved
Fix Postgres Runner disposable procedure -there are no more thrown ex…
9ff736d 
…ception nor custom finalization; Postgres server start process is not disposable anymore cause it is one time call to pg_ctrl; Postgres server stop process is not throwing exceptions anymore - it should report only own output buffer;
Added possibility so specify database which will be used to build con…
84642d7 
…nection string
PostgresRunner doesn't use try/catch block in constructor anymore; in…
160586e 
…stead Start() function is responsible for constructing and preparing running PostgresRunner instance; in case when running Postgres failed then PostgresRunner instance should be disposed.
@zabrowarnyrafal
Copy link
Member Author

Ad1- postponed
Ad2 - done
Ad3- postponed
Ad4 - done
Ad5 - done (but it will be much improved in next releases)
Ad5 - done
Ad6 - done
Ad7 - postponed
Ad8 - postponed
Ad9 - params object - postponed

@zabrowarnyrafal zabrowarnyrafal merged commit d78db9d into bt-skyrise:develop Feb 1, 2018
zabrowarnyrafal added a commit that referenced this pull request Feb 4, 2018
@zabrowarnyrafal
Release of MVP (#21)
b313eee 
* Prepared working solution of Postgres that can be used with integration tests (#1)

* PortWatcher implementation

* PostgresBinaryLocator implementation

* Move code one level up in repo

* PostgresRunner implementation

* Introduced Postgresinitializator process starter; throw exception when process return error code; bum dotnet standard to v2.0; setup basic test

* distinguish pg executables based on os platform; fix long polling of free tcp port

* Added missed binaries of postgres (3 os platforms supported by net core)

* Updated linux based postgres binaries

* Provided sample test suit how to execute pgrunner inside of xunit class fixture

* Removed OSX binaries; OSX as OS Platform won't be supported till issue of it binaries size will be resolved

* Fix Postgres Runner disposable procedure -there are no more thrown exception nor custom finalization; Postgres server start process is not disposable anymore cause it is one time call to pg_ctrl; Postgres server stop process is not throwing exceptions anymore - it should report only own output buffer;

* Added possibility so specify database which will be used to build connection string

* PostgresRunner doesn't use try/catch block in constructor anymore; instead Start() function is responsible for constructing and preparing running PostgresRunner instance; in case when running Postgres failed then PostgresRunner instance should be disposed.

* Provided xunit.runner.visualstudio to Postgres2Go.Samples so tests  can be run directly from IDE (vs or rider)

* Corrected licensing (#18)

Corrected licensing

* #15 use options object in PostgresRunner API

* #15 use options object in PostgresRunner API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcinbudny marcinbudny marcinbudny approved these changes

@pblachut pblachut Awaiting requested review from pblachut

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zabrowarnyrafal @marcinbudny