Skip to content

Conversation

@daveomcd
Copy link
Contributor

@daveomcd daveomcd commented Feb 4, 2020

I took a shot at correcting the tests. Here are the current results, I didn't finish the Active Record tests as I'm not knowledgable enough to see what next steps should be taken place there.

Results from bundle exec rake test ONLY_SQLSERVER=1

Finished in 15.071858s, 15.5920 runs/s, 95.6750 assertions/s.
235 runs, 1442 assertions, 0 failures, 0 errors, 1 skips

Results from bundle exec rake test ONLY_ACTIVERECORD=1

Finished in 160.342181s, 32.8236 runs/s, 93.5562 assertions/s.
5263 runs, 15001 assertions, 0 failures, 16 errors, 3 skips

Copy link
Contributor

@lcreid lcreid left a comment

Choose a reason for hiding this comment

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

This is awesome work. Thanks for fixing all these tests. I don't have any official standing in this gem, but I'm hoping my review gets the attention of the powers that be so they consider merging this PR.

Aside from my one question in the comments, I do have one concern: I only noticed this error a few weeks ago. I did some investigation in recent MiniTest releases, and it looks to me like we should have been seeing this error perhaps since last September? I could be wrong about that.

At any rate I'm curious about whether it was the change to MiniTest, or something has changed in the way ActiveRecord or the SQL Server Adapter works, and if so, is it an indication that it's not the tests that were broken, but rather the code? I'm hoping someone with more experience with this gem can weigh in.

I'm going to update your ticket with a few more thoughts related to the above.

But again, great work overall. Thanks for the PR.

@wpolicarpo
Copy link
Member

@daveomcd do you mind rebasing your branch and resolve the conflicts? I merged #718, which contained must of the fixes in here.

@daveomcd
Copy link
Contributor Author

daveomcd commented Mar 2, 2020

@daveomcd do you mind rebasing your branch and resolve the conflicts? I merged #718, which contained must of the fixes in here.

My apologies on being a complete newb with Git. How can I retrieve the conflicts I see below to resolve and push back up ?

@lcreid
Copy link
Contributor

lcreid commented Mar 3, 2020

@daveomcd you need to:

  1. Update your repo from the original rails-sqlserver repo. This is typically called the upstream repo. https://www.atlassian.com/git/tutorials/git-forks-and-upstreams has a pretty good description of how to do this
  2. Rebase or merge your correcting-tests branch with upstream/master (assuming you called the rails-sqlserver repo upstream). The link above describes how to update your master branch. To update your correcting-tests branch, just do the same thing, but in the correcting-tests branch
  3. Resolve the merge conflicts. This typically means going through each file that has merge conflicts in your editor or IDE and manually correcting the files to what they should be. Hopefully in this case that should be obvious
  4. Commit all the files
  5. Push your correcting-tests branch to your GitHub repo, which will automatically update your PR

I hope this helps, but if not just keep asking questions. We're happy to help.

@wpolicarpo
Copy link
Member

Hey @daveomcd can I ask you to rebase this and resolve the conflicts so I can merge?

@daveomcd
Copy link
Contributor Author

Hey @daveomcd can I ask you to rebase this and resolve the conflicts so I can merge?

My apologies I'll try working on it now. Things have been crazy! :(

@daveomcd
Copy link
Contributor Author

daveomcd commented Mar 27, 2020

Hey @daveomcd can I ask you to rebase this and resolve the conflicts so I can merge?

So... @wpolicarpo, I followed through everything and now I'm getting this message.

Applying: corrected syntax for DEPRECATED warnings in test/cases
No changes - did you forget to use 'git add'?
If there is nothing left to stage, chances are that something else
already introduced the same changes; you might want to skip this patch.

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

When I run git rebase --continue. Does this just mean I took so long that someone else already incorporated these changes? Ha. If that's the case my sincerest apologies!

@wpolicarpo
Copy link
Member

@daveomcd don't be sorry. You're helping and we need to thank you for that.

First thing to do is to update your upstream master branch as explained in #725 (comment).

Some changes in your PR were already merged into master indeed, but there are other we can still integrate. So you need to git rebase -i master and resolve the conflicts as they appear.

For each step after resolving the conflicts you will need to add your changed back git add file_1 file_2 and finally git rebase --continue to end up with a clean rebased tree.

If you prefer, you can merge instead of rebasing if it is easier to understand for now.

@daveomcd
Copy link
Contributor Author

daveomcd commented Mar 27, 2020

@daveomcd don't be sorry. You're helping and we need to thank you for that.

First thing to do is to update your upstream master branch as explained in #725 (comment).

Some changes in your PR were already merged into master indeed, but there are other we can still integrate. So you need to git rebase -i master and resolve the conflicts as they appear.

For each step after resolving the conflicts you will need to add your changed back git add file_1 file_2 and finally git rebase --continue to end up with a clean rebased tree.

If you prefer, you can merge instead of rebasing if it is easier to understand for now.

So @wpolicarpo, I believe I did many of those steps.

git remote add upstream https://github.com/rails-sqlserver/activerecord-sqlserver-adapter.git
git fetch upstream
git checkout correcting-tests
git rebase upstream/master

Which gave me the following...

First, rewinding head to replay your work on top of it...
Applying: corrected syntax for DEPRECATED warnings in test/cases
Using index info to reconstruct a base tree...
M       test/cases/adapter_test_sqlserver.rb
M       test/cases/change_column_null_test_sqlserver.rb
M       test/cases/coerced_tests.rb
M       test/cases/column_test_sqlserver.rb
M       test/cases/connection_test_sqlserver.rb
M       test/cases/fetch_test_sqlserver.rb
M       test/cases/json_test_sqlserver.rb
M       test/cases/migration_test_sqlserver.rb
M       test/cases/pessimistic_locking_test_sqlserver.rb
M       test/cases/rake_test_sqlserver.rb
M       test/cases/schema_dumper_test_sqlserver.rb
M       test/cases/schema_test_sqlserver.rb
M       test/cases/showplan_test_sqlserver.rb
M       test/cases/specific_schema_test_sqlserver.rb
M       test/cases/transaction_test_sqlserver.rb
M       test/cases/trigger_test_sqlserver.rb
M       test/cases/utils_test_sqlserver.rb
M       test/cases/uuid_test_sqlserver.rb
Falling back to patching base and 3-way merge...
Auto-merging test/cases/transaction_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/transaction_test_sqlserver.rb
Auto-merging test/cases/specific_schema_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/specific_schema_test_sqlserver.rb
Auto-merging test/cases/schema_dumper_test_sqlserver.rb
Auto-merging test/cases/rake_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/rake_test_sqlserver.rb
Auto-merging test/cases/column_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/column_test_sqlserver.rb
Auto-merging test/cases/coerced_tests.rb
error: Failed to merge in the changes.
Patch failed at 0001 corrected syntax for DEPRECATED warnings in test/cases
Use 'git am --show-current-patch' to see the failed patch

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

I corrected them all then did...

git add .
git commit
git rebase --continue

Which resulted in...

Applying: corrected syntax for DEPRECATED warnings in test/cases
No changes - did you forget to use 'git add'?
If there is nothing left to stage, chances are that something else
already introduced the same changes; you might want to skip this patch.

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

@tbonz
Copy link

tbonz commented Mar 27, 2020

@daveomcd it looks like you added a git commit in there instead of just continuing with the rebase after you added your corrections. You only needed to:

git add .
git rebase --continue

You might reference this issue to see how you can reset your branch and allow you to continue with the rebase. https://stackoverflow.com/a/12163247/1245332

@daveomcd
Copy link
Contributor Author

daveomcd commented Mar 27, 2020

@daveomcd it looks like you added a git commit in there instead of just continuing with the rebase after you added your corrections. You only needed to:

git add .
git rebase --continue

You might reference this issue to see how you can reset your branch and allow you to continue with the rebase. https://stackoverflow.com/a/12163247/1245332

@tbonz thanks for the help. So I'm trying to get this going in between work. I attempted to resolve the issue by doing the following but still seeing the same results.

git reset --soft HEAD^
git rebase --continue

Applying: corrected syntax for DEPRECATED warnings in test/cases
Applying: corrected syntax for DEPRECATED warnings in test/cases
Using index info to reconstruct a base tree...
M       Gemfile
M       test/cases/adapter_test_sqlserver.rb
M       test/cases/change_column_null_test_sqlserver.rb
M       test/cases/coerced_tests.rb
M       test/cases/column_test_sqlserver.rb
M       test/cases/connection_test_sqlserver.rb
M       test/cases/fetch_test_sqlserver.rb
M       test/cases/json_test_sqlserver.rb
M       test/cases/migration_test_sqlserver.rb
M       test/cases/pessimistic_locking_test_sqlserver.rb
M       test/cases/rake_test_sqlserver.rb
M       test/cases/schema_dumper_test_sqlserver.rb
M       test/cases/schema_test_sqlserver.rb
M       test/cases/showplan_test_sqlserver.rb
M       test/cases/specific_schema_test_sqlserver.rb
M       test/cases/transaction_test_sqlserver.rb
M       test/cases/trigger_test_sqlserver.rb
M       test/cases/utils_test_sqlserver.rb
M       test/cases/uuid_test_sqlserver.rb
Falling back to patching base and 3-way merge...
Auto-merging test/cases/transaction_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/transaction_test_sqlserver.rb
Auto-merging test/cases/specific_schema_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/specific_schema_test_sqlserver.rb
Auto-merging test/cases/rake_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/rake_test_sqlserver.rb
Auto-merging test/cases/column_test_sqlserver.rb
CONFLICT (content): Merge conflict in test/cases/column_test_sqlserver.rb
error: Failed to merge in the changes.
Patch failed at 0002 corrected syntax for DEPRECATED warnings in test/cases
Use 'git am --show-current-patch' to see the failed patch

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

At this point I went and corrected the conflicts and then proceeded with the following.

git status

rebase in progress; onto 20b0f60
You are currently rebasing branch 'correcting-tests' on '20b0f60'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Unmerged paths:
  (use "git reset HEAD <file>..." to unstage)
  (use "git add <file>..." to mark resolution)

        both modified:   test/cases/column_test_sqlserver.rb
        both modified:   test/cases/rake_test_sqlserver.rb
        both modified:   test/cases/specific_schema_test_sqlserver.rb
        both modified:   test/cases/transaction_test_sqlserver.rb

no changes added to commit (use "git add" and/or "git commit -a")
git add .
git rebase --continue

Then seeing the same no changes message below.

Applying: corrected syntax for DEPRECATED warnings in test/cases
No changes - did you forget to use 'git add'?
If there is nothing left to stage, chances are that something else
already introduced the same changes; you might want to skip this patch.

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

I promise once I've graduated to the point past absolute newb at git I'll be able to contribute to more issues here without so many problems!

Updated:

Here's my git log as well if any of this provides context that is helpful.

commit e4cfb975275729b46b627fd1e203f1c1e5b81e8c (HEAD)
Author: David McDonald <daveomcd@gmail.com>
Date:   Thu Nov 7 11:44:00 2019 -0500

    corrected syntax for DEPRECATED warnings in test/cases

commit 20b0f6071603abe040894b5f7772a8c53d01f340 (upstream/master)
Merge: 75ebf70 72737e8
Author: Wanderson Policarpo <wpolicarpo@gmail.com>
Date:   Wed Mar 25 11:56:40 2020 +0000

    Merge pull request #745 from aidanharan/fix-ar-relations-tests

    Quoted table names containing square brackets need to be regex escaped

commit 72737e8c07d84675559f215ec10c74684d08ec96
Author: Aidan Haran <aidanharan@yahoo.com>
Date:   Tue Mar 24 17:08:19 2020 +0000

    Quoted table names containing square brackets need to be regex escaped

    Use method from Rails master

@tbonz
Copy link

tbonz commented Mar 27, 2020

@daveomcd Can you try adding the files individually after saving them? It seems like the files are not getting staged before you continue. Maybe you try:

git add test/cases/column_test_sqlserver.rb
git status

and confirm the file is getting staged before you git rebase --continue

@daveomcd
Copy link
Contributor Author

daveomcd commented Mar 29, 2020

@daveomcd Can you try adding the files individually after saving them? It seems like the files are not getting staged before you continue. Maybe you try:

git add test/cases/column_test_sqlserver.rb
git status

and confirm the file is getting staged before you git rebase --continue

@tbonz, Well the files that I need to resolve conflicts on I'm taking the "HEAD" changes each time. So that makes me think the message is correct, that there doesn't appear to be any changes. The conflicts were spots where they master uses paranthesis and I don't for my method arguments, etc.

@tbonz
Copy link

tbonz commented Mar 31, 2020

@daveomcd I was able to clone down your fork and successfully rebase onto the upstream master. I might recommend cloning your project down to a new directory and follow the steps again to rebase. Here's what I did..

tbonz:~/dev
$ git clone git@github.com:daveomcd/activerecord-sqlserver-adapter.git
tbonz:~/dev
$ cd activerecord-sqlserver-adapter/
tbonz:~/dev/activerecord-sqlserver-adapter git:(master)
$ git remote add upstream git@github.com:rails-sqlserver/activerecord-sqlserver-adapter.git
tbonz:~/dev/activerecord-sqlserver-adapter git:(master)
$ git fetch upstream
tbonz:~/dev/activerecord-sqlserver-adapter git:(master)
$ git checkout correcting-tests
tbonz:~/dev/activerecord-sqlserver-adapter git:(𝘮 ← 2 correcting-tests)
$ git rebase upstream/master
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 4B
$ git status
rebase in progress; onto b833393
You are currently rebasing branch 'correcting-tests' on 'b833393'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   Gemfile
	modified:   test/cases/coerced_tests.rb
	modified:   test/cases/schema_dumper_test_sqlserver.rb

Unmerged paths:
  (use "git reset HEAD <file>..." to unstage)
  (use "git add <file>..." to mark resolution)

	both modified:   test/cases/column_test_sqlserver.rb
	both modified:   test/cases/rake_test_sqlserver.rb
	both modified:   test/cases/specific_schema_test_sqlserver.rb
	both modified:   test/cases/transaction_test_sqlserver.rb

Open editor and fix any conflicts (accept HEAD changes as you mentioned) in each file, and save!

tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 4B
$ git add test/cases/column_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 3B
$ git add test/cases/rake_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 2B
$ git add test/cases/specific_schema_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 1B
$ git add test/cases/transaction_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M
$ git status
rebase in progress; onto b833393
You are currently rebasing branch 'correcting-tests' on 'b833393'.
  (all conflicts fixed: run "git rebase --continue")

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   Gemfile
	modified:   test/cases/coerced_tests.rb
	modified:   test/cases/schema_dumper_test_sqlserver.rb

tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M
$ git rebase --continue
Applying: corrected syntax for DEPRECATED warnings in test/cases
Applying: corrected syntax for DEPRECATED warnings in test/cases
tbonz:~/dev/activerecord-sqlserver-adapter git:(𝘮 ← 2 correcting-tests 2⇵20)
$ git status
On branch correcting-tests
Your branch and 'origin/correcting-tests' have diverged,
and have 20 and 2 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

nothing to commit, working tree clean

The next step would be to force push this back to origin.

tbonz:~/dev/activerecord-sqlserver-adapter git:(𝘮 ← 2 correcting-tests 2⇵20)
$ git push -f origin correcting-tests

Let me know if this helps, or I could offer a meetup on google to assist any further.

@daveomcd
Copy link
Contributor Author

@daveomcd I was able to clone down your fork and successfully rebase onto the upstream master. I might recommend cloning your project down to a new directory and follow the steps again to rebase. Here's what I did..

tbonz:~/dev
$ git clone git@github.com:daveomcd/activerecord-sqlserver-adapter.git
tbonz:~/dev
$ cd activerecord-sqlserver-adapter/
tbonz:~/dev/activerecord-sqlserver-adapter git:(master)
$ git remote add upstream git@github.com:rails-sqlserver/activerecord-sqlserver-adapter.git
tbonz:~/dev/activerecord-sqlserver-adapter git:(master)
$ git fetch upstream
tbonz:~/dev/activerecord-sqlserver-adapter git:(master)
$ git checkout correcting-tests
tbonz:~/dev/activerecord-sqlserver-adapter git:(𝘮 ← 2 correcting-tests)
$ git rebase upstream/master
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 4B
$ git status
rebase in progress; onto b833393
You are currently rebasing branch 'correcting-tests' on 'b833393'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   Gemfile
	modified:   test/cases/coerced_tests.rb
	modified:   test/cases/schema_dumper_test_sqlserver.rb

Unmerged paths:
  (use "git reset HEAD <file>..." to unstage)
  (use "git add <file>..." to mark resolution)

	both modified:   test/cases/column_test_sqlserver.rb
	both modified:   test/cases/rake_test_sqlserver.rb
	both modified:   test/cases/specific_schema_test_sqlserver.rb
	both modified:   test/cases/transaction_test_sqlserver.rb

Open editor and fix any conflicts (accept HEAD changes as you mentioned) in each file, and save!

tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 4B
$ git add test/cases/column_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 3B
$ git add test/cases/rake_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 2B
$ git add test/cases/specific_schema_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M 1B
$ git add test/cases/transaction_test_sqlserver.rb
tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M
$ git status
rebase in progress; onto b833393
You are currently rebasing branch 'correcting-tests' on 'b833393'.
  (all conflicts fixed: run "git rebase --continue")

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   Gemfile
	modified:   test/cases/coerced_tests.rb
	modified:   test/cases/schema_dumper_test_sqlserver.rb

tbonz:~/dev/activerecord-sqlserver-adapter git:(upstream ⚡ detached@b833393) 3M
$ git rebase --continue
Applying: corrected syntax for DEPRECATED warnings in test/cases
Applying: corrected syntax for DEPRECATED warnings in test/cases
tbonz:~/dev/activerecord-sqlserver-adapter git:(𝘮 ← 2 correcting-tests 2⇵20)
$ git status
On branch correcting-tests
Your branch and 'origin/correcting-tests' have diverged,
and have 20 and 2 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

nothing to commit, working tree clean

The next step would be to force push this back to origin.

tbonz:~/dev/activerecord-sqlserver-adapter git:(𝘮 ← 2 correcting-tests 2⇵20)
$ git push -f origin correcting-tests

Let me know if this helps, or I could offer a meetup on google to assist any further.

@tbonz oh man you're the best. Trying it now. Thanks so much for taking the time to teach me how to do this.

@daveomcd
Copy link
Contributor Author

@tbonz thanks! I think that worked. Only thing I noticed was I could probably consolidate my commits in my log. But I didn't due to I was nervous I'd screw it up again! But I can't do that next if necessary. @wpolicarpo thank you as well for helping me out. I look forward to trying to help out to another area and getting this process down.

@wpolicarpo wpolicarpo merged commit 6ea5654 into rails-sqlserver:master Apr 8, 2020
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.

4 participants