CollaboratingPlatypus / PetaPoco

Official PetaPoco, A tiny ORM-ish thing for your POCO's
Other
2.07k stars 600 forks source link

Fix remaining failing integration tests #710

Closed Curlack closed 6 months ago

Curlack commented 1 year ago

Test Oracle Delimited and Ordinary Identifiers Separately.

IntegrationTests

Split Oracle DB scripts, connection strings and tests in two. One set of tests to use PP OOTB (Delimited) and one set of tests to use PP with Custom provider and mapper (Ordinary). Fix Oracle setup script. Wait for tablespace drop operation to complete before creating the new tablespace.

Core

Database.cs

OracleDatabaseProvider.cs

asherber commented 1 year ago

I know this PR is only going to an internal branch right now, not to develop, but I think the changes to PocoData should be their own PR -- and should stay that way going into develop. That's too significant a plumbing change to be buried in a PR about Oracle tests. Best way to handle is probably to do that on its own in a separate branch, and then once its merged to develop merge develop into this branch.

Curlack commented 1 year ago

I know this PR is only going to an internal branch right now, not to develop, but I think the changes to PocoData should be their own PR -- and should stay that way going into develop. That's too significant a plumbing change to be buried in a PR about Oracle tests. Best way to handle is probably to do that on its own in a separate branch, and then once its merged to develop merge develop into this branch.

I can make the changes on develop and create a PR. How would you like to handle the changes in this PR though, first merge into develop and then pull into oracle-support?

asherber commented 1 year ago

The PocoData changes should be on their own branch with their own PR and then merged to develop if they're accepted. Then back out the PocoData changes from this branch, force push, and merge develop into this branch.

asherber commented 1 year ago

Also, sorry to nitpick, but the first line of a git commit message should just be a brief (50 char) summary of what the commit does; the rest can follow after a blank line. This is because many git clients by default only display the first line of the message. See https://cbea.ms/git-commit/ for some good guidelines.

Ste1io commented 1 year ago

I'm just sitting down to catch up on everything, been a hectic few days, but happy to see momentum is continuing at such a good clip.

Ste1io commented 1 year ago

@Curlack, I'm preparing a discussion post I'll be posting here in the next few hours that directly addresses quoting behavior and identifier handling. Just wanted to let you know to avoid any possible repeated work and ensure we're on the same page in how it is handled. It's taken longer than I planned due to my absence, but also several other nuances came to the surface yesterday that factor in as well, that haven't been brought up and if not discussed now, will result in a lot of backtracking and recoding.

Curlack commented 1 year ago

I've pushed the last changes to the remaining failing oracle tests. These tests will only pass once we have the PocoData changes (pending development branch PR).

Curlack commented 1 year ago

Wanting to flex my new skills...any objections to me squashing these commits into one?

asherber commented 1 year ago

Go for it!

Note that it's not always appropriate to squash commits. A commit should be a unit of work that has some significance. You might start out with a branch that has 6 or 7 commits, because you're following the maxim of committing often, and you might decide that really you've done two distinct things on that branch -- so you can squash down to two commits rather than one.

Curlack commented 1 year ago

Understood. Thanks.

asherber commented 1 year ago

Also, you shouldn't squash or otherwise change commits that have already been merged somewhere else.

One usual use case is to make local commits and squash them before pushing. Another, like this, is to have a series of commits in a pull request that get squashed before merging. GitHub can actually do that itself, from the web interface, but as the committer you have more control if you do it yourself.

pleb commented 12 months ago

@Curlack is this PR ready to merge?

Curlack commented 12 months ago

I think so. Not sure if @Ste1io has some more changes.

Ste1io commented 12 months ago

@Curlack is this PR ready to merge?

I'd like to hold off till @Curlack and I can iron a couple things out with the current implementation. Been away a good bit over the Thanksgiving holiday, but will be back in the swing of things here heading into the weekend; appreciate your patience guys.

Ste1io commented 11 months ago

@pleb @asherber @Curlack, I apologize for my extended absence over the holidays, and appreciate all of your patience.

@Curlack, I've merged the changes in the development branch (containing your last couple of PRs) into the public feature branch this PR is based on (upstream feature/oracle-support). Since this is your private feature branch, you'll want to sync it up with a rebase/force push. You can either do it locally by fetching it from upstream then force-pushing it to your origin, or you can use the option below this comment here on github, choosing the rebase option (not merge):

1703920626-CollaboratingPlatypusPetaPoco_at_featureoracle-sup

Following that, you'll need to pull the changes to your local from your origin (since your origin is what is being rebased using the update branch button.

I'll follow up this comment with another addressing this PR specifically, but wanted to get our feature branch up to date with development before anything else.

asherber commented 11 months ago

Since there no conflicts between @Curlack 's branch and the base branch, I would recommend not rebasing, unless there's reason to think that recent changes in development will have a functional impact on the changes in this PR. From a code perspective, merging this PR will work just fine without a rebase.

cc: @Ste1io

pleb commented 6 months ago

Apologies, I missed this one.