Skip to content

Multi-version testing - #929

Merged
austindrenski merged 7 commits into
npgsql:devfrom
austindrenski:azure-pipelines-multi-postgres
Jul 25, 2019
Merged

Multi-version testing#929
austindrenski merged 7 commits into
npgsql:devfrom
austindrenski:azure-pipelines-multi-postgres

Conversation

@austindrenski

@austindrenski austindrenski commented Jul 18, 2019

Copy link
Copy Markdown
Contributor

Not sure if this is going to work yet, but Azure Pipelines offers 10 parallel jobs, so why not give it a try.

This almost works right now, but the standard PostgreSQL image (obviously) doesn't include PostGIS, so we need to work out how to add that to it.

Related

#646
#928
#934

@austindrenski austindrenski self-assigned this Jul 18, 2019
@austindrenski
austindrenski force-pushed the azure-pipelines-multi-postgres branch from 7c77136 to 667c650 Compare July 18, 2019 23:59
@austindrenski
austindrenski force-pushed the azure-pipelines-multi-postgres branch from 667c650 to 50ca047 Compare July 19, 2019 00:25
@roji

roji commented Jul 19, 2019

Copy link
Copy Markdown
Member

I'll give this a review when I have time, but I still think this would add more trouble than it would save. See #710 and npgsql/npgsql#2477 (comment)

Comment thread azure-pipelines.yml Outdated

- bash: sudo sh .build/setup_postgres.sh
- bash: |
docker exec -d $[variables['postgres_service']] apt-get -y install postgresql-11-postgis-2.5

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought @austindrenski: I wonder if getting PG via docker can't speed up our build regardless of multi-version testing. I can see that PostgreSQL installation takes around 1 minute 40 seconds in current builds, maybe this could be sped up?

Then again, we'd have wire up all the proper extensions/configuration (PostGIS, SSL, prepared transactions...), not sure it's worth the bother.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had the same thought. In the long run, I think it's worthwhile to at least kick the tires here.

@austindrenski austindrenski Jul 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docker startup time ~35 seconds. So, definitely some value here.

For now, we're only getting this benefit on the Linux job, as it appears that AP doesn't currently support using Linux service container from a Windows agent job. But we might get to sidestep that altogether by building the jobs within containers too. Further testing needed, though.

Take a look at build 149 build 150 for a good example. It failed, but only because our test suite isn't super version-aware at the moment.

edit: Windows build failed on 149 due to a bad script

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going the Docker route, the Linux build time drops to 5:46 of which 4:20 was test execution. Whereas the windows side finishes in 12:08 of which 8:52 was test execution.

It doesn't seem like there's an off-the-shelf image for what we need (e.g. Windows + PostgreSQL + PostGIS), so containerizing the whole thing might require us to layer our own image based on something like https://github.com/appropriate/docker-postgis.

I think there's some upside to exploring this further, including future proofing us against future CI changes, and making it easier to reproduce the exact CI environment locally.

(Still all conditional on the maintenance/usefulness trade-off, and our ability to avoid building all of the images from scratch.)

@austindrenski
austindrenski force-pushed the azure-pipelines-multi-postgres branch 22 times, most recently from 9d51230 to afb5892 Compare July 24, 2019 01:48
@austindrenski
austindrenski force-pushed the azure-pipelines-multi-postgres branch 3 times, most recently from 7f97a2f to 6106a54 Compare July 24, 2019 06:41
@austindrenski
austindrenski marked this pull request as ready for review July 24, 2019 06:51
@austindrenski

Copy link
Copy Markdown
Contributor Author

TODO:

  1. Setup releases to NuGet.
  2. Investigate containerizing the Windows build.

Comment thread test/EFCore.PG.FunctionalTests/Query/FullTextSearchDbFunctionsNpgsqlTest.cs Outdated
@austindrenski
austindrenski force-pushed the azure-pipelines-multi-postgres branch 3 times, most recently from 580d64b to 2cf729b Compare July 25, 2019 02:58
@austindrenski

Copy link
Copy Markdown
Contributor Author

Planning to merge this in the next hour. There's a few files that conflict with #943, but I've attempted to cluster those changes to the last commit (e.g. 2cf729b).

- Fixes for PG 9.6 and 10 (partial)
- GitHub releases
- Pack regular NuGet packages (i.e. non-CI version numbers).
- Turns out that Build.{Artifact,}StagingDirectory are the same,
  so going with the shorter for clarity.
This may cause conflicts with npgsql#943
@austindrenski
austindrenski force-pushed the azure-pipelines-multi-postgres branch from 2cf729b to 345445f Compare July 25, 2019 03:17
@austindrenski
austindrenski merged commit ac1ff06 into npgsql:dev Jul 25, 2019
@austindrenski
austindrenski deleted the azure-pipelines-multi-postgres branch July 25, 2019 03:34
Comment thread test/EFCore.PG.FunctionalTests/Query/NetworkQueryNpgsqlTest.cs
@roji

roji commented Jul 25, 2019

Copy link
Copy Markdown
Member

One more idea: why don't we starting running tests on PostgreSQL 12 beta1 as well? It will allow us to start working on new PG 12 features (I'm thinking about generated columns in particular), and would in theory flush out any breaking changes on the PG side.

@roji roji mentioned this pull request Jul 25, 2019
@austindrenski

Copy link
Copy Markdown
Contributor Author

Testing against the betas sounds like an excellent idea. I'll look into wiring that up as part of phase 3.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants