Help resolving master test errors

[daveomcd]

I'm currently attempting to get the master branch tests to work as there are a bunch of tests that are failing. I have managed to resolve many of the errors in test/cases folder, but it appears the errors I'm now getting are mostly ActiveRecord tests from coerced_tests.rb that haven't been implemented. My knowledge on how to resolve some of these at this point is very limited due to I've not collaborated on many projects. What's the best way to share my current cloned project changes to allow others in assisting fixing these? Creating a pull request?

Current Status

$ bundle exec rake test ONLY_SQLSERVER=1

Finished in 13.108786s, 17.9269 runs/s, 110.0026 assertions/s.
235 runs, 1442 assertions, 0 failures, 0 errors, 1 skips

$ bundle exec rake test ONLY_ACTIVERECORD=1

Finished in 138.995701s, 37.7494 runs/s, 107.3702 assertions/s.
5247 runs, 14924 assertions, 0 failures, 13 errors, 3 skips

An example of a test I tried to correct from one of the 13 errors mentioned above would be the one below:

  coerce_tests! :test_pluck_with_from_includes_quoted_original_table_name
  def test_pluck_with_from_includes_quoted_original_table_name_coerced
    relations = Post.joins(:author).order(:id)
    subquery = Post.from("#{Post.quoted_table_name} /*! USE INDEX (PRIMARY) */").includes(:author).order(:id)
    assert_equal relations.pluck(:id), subquery.pluck(:id)
  end

Error:
RelationTest#test_pluck_with_from_includes_quoted_original_table_name_coerced:
ActiveRecord::StatementInvalid: TinyTds::Error: Ambiguous column name 'id'.: SELECT [id] FROM [posts] /*! USE INDEX (PRIMARY) */ LEFT OUTER JOIN [authors] ON [authors].[id] = [posts].[author_id]  ORDER BY [id] ASC

bin/rails test mnt/c/Users/mcdonaldd/Documents/open_source/sqlserver-main/test/cases/coerced_tests.rb:763

Now to my knowledge I'd expect having to change the following line...

assert_equal relations.pluck(:id), subquery.pluck(:id)

to

assert_equal relations.pluck(:id), subquery.pluck('authors.id')

But, I'm not sure if that really tests for what it's intended to test for... So this is where I'm currently stuck.

Actual Questions:

I've covered a couple of things here so I'll summarize into these two questions...

1. What's the best way for me to share my current changes so that others might be able to assist me in correcting these tests? Can I do a pull request with the changes even though nothing is really totally resolved?

2. What would be the approach to fixing the new test I added: RelationTest#test_pluck_with_from_includes_quoted_original_table_name_coerced

As I would expect the Ambiguous column 'id' name error if I had not specified which table the id column was supposed to come from...

[ttilberg]

What's the best way for me to share my current changes so that others might be able to assist me in correcting these tests? Can I do a pull request with the changes even though nothing is really totally resolved?

This repository hasn't been very active lately, but I do think this is the best way to get feedback. Even if the PR isn't ready, you should mark it was a WIP: in the title and push it here. I think it stands the best opportunity to get more eyes on it that way. If the maintainers want to close the PR, perhaps they can give better guidance at that time how to proceed. But in the meantime, maybe we can get a few more of those tests passing. I very much would like to see SQL Server + Rails 6. Thanks for trying to work on this.

[lcreid]

@daveomcd I've opened #722 with some additional information. If #722 is a complete duplicate of your ticket, I apologize.

Did you post a PR for this issue? I'm going to try to fix #722 but I don't want to duplicate your efforts.

[daveomcd]

@lcreid Hey Larry thanks, my apologies I forgot to post a PR. Mainly because I've never done it before besides minor changes in which I completed through the Github website. Looking at how to push up my local changes now...

Update: Not sure if this is the place to discuss... but reading up on how to post a PR with my local changes, sites have mentioned that I should push my local changes to a repo on my account. I'm currently trying to do that, but it seems it is trying to push to this account instead of my own...

daveomcd@mcdonald-precis:~/open_source/sqlserver-main$ git config --get remote.origin.url 
https://github.com/daveomcd/activerecord-sqlserver-adapter.git
daveomcd@mcdonald-precis:~/open_source/sqlserver-main$ git push origin
fatal: The current branch correcting-tests has no upstream branch.
To push the current branch and set the remote as upstream, use

git push --set-upstream origin correcting-tests

daveomcd@mcdonald-precis:~/open_source/sqlserver-main$ git push --set-upstream origin correcting-tests
Username for 'https://github.com': daveomcd
Password for 'https://daveomcd@github.com':
remote: Permission to rails-sqlserver/activerecord-sqlserver-adapter.git denied to daveomcd.
fatal: unable to access 'https://github.com/rails-sqlserver/activerecord-sqlserver-adapter.git/': The requested URL returned error: 403

[dglaser]

@daveomcd Have you resolved the issue with pushing your changes to your repos?

You could try declaring another remote and pushing to it: git remote add mine https://github.com/daveomcd/activerecord-sqlserver-adapter.git git push --set-upstream mine correcting-tests

You can also add -v to show more information about what is happening: git push -v --set-upstream origin correcting-tests

[daveomcd]

@dglaser thanks! That worked. I really appreciate the help from everyone. I hope my changes weren't a let down :(

@lcreid, sorry for the delay in getting this up.

https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/725

[lcreid]

A few weeks ago I was debugging some of the test failures that were getting in the way of the Rails 6 version of this adapter. During that time, I never saw these errors. I submitted a PR to Rails to fix the errors I was looking at, and went back to working on the adapter, and started seeing these errors.

In https://github.com/seattlerb/minitest/commit/15ed8e4ce504c8313058a1d6fc4918299be34328, documentation was enhanced to cover the test failures we were seeing, which makes me think that the MiniTest changes had happened earlier. I can't find when the change that would have caused this error to only start appearing to me 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, or even earlier? I could be wrong about that. At any rate, maybe it's just me that didn't see them.

The real concern I have is if this isn't caused by the change to MiniTest, or but rather if 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. @metaskills @wpolicarpo ?

Sorry I can't be more helpful at the moment.

[aidanharan]

The master tests are now passing so this issue can probably be closed.

[wpolicarpo]

Since v5.2.1 was released and master tests are green-ish (some Rails tests are still failing in appveyor), I'll close this off.