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

Fix remaining failing integration tests #710

Conversation

Curlack
Copy link
Contributor

@Curlack Curlack commented Nov 1, 2023

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

  • Escape Oracle Stored Procedure parameter names instead of ensuring paramater prefix.

OracleDatabaseProvider.cs

  • Quote identifier when escaping (no Regex or ToUpperInvariant).
  • Disable statement caching for Oracle client.

@asherber
Copy link
Collaborator

asherber commented Nov 1, 2023

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.

PetaPoco/Core/PocoData.cs Outdated Show resolved Hide resolved
@Curlack
Copy link
Contributor Author

Curlack commented Nov 1, 2023

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
Copy link
Collaborator

asherber commented Nov 2, 2023

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
Copy link
Collaborator

asherber commented Nov 2, 2023

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
Copy link
Collaborator

Ste1io commented Nov 2, 2023

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
Copy link
Collaborator

Ste1io commented Nov 5, 2023

@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
Copy link
Contributor Author

Curlack commented Nov 5, 2023

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
Copy link
Contributor Author

Curlack commented Nov 6, 2023

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

@asherber
Copy link
Collaborator

asherber commented Nov 6, 2023

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
Copy link
Contributor Author

Curlack commented Nov 6, 2023

Understood. Thanks.

@asherber
Copy link
Collaborator

asherber commented Nov 6, 2023

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.

@Curlack Curlack changed the title Fix remaining failing integration tests for #699. Fix remaining failing integration tests Nov 6, 2023
#### 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**
- Escape Oracle Stored Procedure parameter names instead of ensuring paramater prefix.

**OracleDatabaseProvider.cs**
- Quote identifier when escaping (no Regex or ToUpperInvariant).
- Disable statement caching for Oracle client.
@pleb
Copy link
Member

pleb commented Nov 30, 2023

@Curlack is this PR ready to merge?

@Curlack
Copy link
Contributor Author

Curlack commented Nov 30, 2023

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

@Ste1io
Copy link
Collaborator

Ste1io commented Nov 30, 2023

@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
Copy link
Collaborator

Ste1io commented Dec 30, 2023

@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
Copy link
Collaborator

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 pleb merged commit 96bf729 into CollaboratingPlatypus:feature/oracle-support May 14, 2024
@pleb
Copy link
Member

pleb commented May 14, 2024

Apologies, I missed this one.

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

4 participants