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

ActiveRecord base test fixes #62

Merged
merged 7 commits into from
Apr 15, 2020

Conversation

alimi
Copy link
Contributor

@alimi alimi commented Apr 10, 2020

This includes a few changes to fix ActiveRecord's base test.

The biggest change is the introduction of a CockroachDB specific test schema. It's a copy of the PostgreSQL test schema defined by ActiveRecord with a few changes so it works for CockroachDB.

See the commits for more details on the changes.

Note: there's an outstanding failure in the base test. test_marshal_between_processes causes a Ruby segfault when it tries to open a new connection in a forked process. I'm not sure what's going there so I'm tabling it for now.

  * CockroachDB uses unique_rowid() for primary keys, not sequences. It's
    possible to force a table to use sequences, but since it's not the default
    behavior we'll always return nil for default_sequence_name.
  * The CockroachDB test schema replaces the PostgreSQL test schema used by the
    ActiveRecord test suite. It's very similiar to the PostgreSQL schema, but
    it removes anything that doesn't work against CockroachDB.
  * Before loading the CockroachDB test schema, we first have to load the
    ActiveRecord test helper. Besides loading dependencies that make the schema
    load work, the AR test helper also loads the default schema used by all
    databases.
  * test/cases/helper_cockroachdb is loaded by every test file and it now loads
    the AR test helper. Therefore, there's no need to explicitly load the AR
    test helper in the test files.
We have to override extract_value_from_default so we can get the
correct time and date defaults for a couple of reasons.

  1) There's a bug in CockroachDB where the date type is missing from the column
     info query. cockroachdb/cockroach#47285
  2) PostgreSQL's timestamp without time zone type maps to CockroachDB's
     TIMESTAMP type. TIMESTAMP includes a UTC time zone while timestamp without
     time zone doesn't. https://www.cockroachlabs.com/docs/v19.2/timestamp.html#variants
@alimi
Copy link
Contributor Author

alimi commented Apr 13, 2020

The Ruby segfault problem might be specific to the pg gem on macOS. Per ged/ruby-pg#311, I could get to the test to run and pass with gssencmode: disable in my database.yml. The test still segfaulted when I ran it with other tests though. We'll have to see if this problem exists in other environments like CI.

h/t @marlabrizel for having seen this problem before 🌟

  * The test fails because the setup doesn't account for other adapters like
    CockroachDBAdapter.
@alimi
Copy link
Contributor Author

alimi commented Apr 13, 2020

Forgot to commit f110047 on Friday.

);
_SQL

# Since stored procedures are not supported in CockroachDB, it won't be possible
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this file is a copy of AR's PostgreSQL specific schema, I left stuff like this in place but commented out thinking it might make it easier to see the differences between the two files. We can take it out if it isn't helpful. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is ok to keep.

@apantel
Copy link
Contributor

apantel commented Apr 14, 2020

Looks good, just making sure the branch is in the state you want.

@alimi
Copy link
Contributor Author

alimi commented Apr 14, 2020

It's ready to merge. :shipit:

  * In addition to handling dates and times, extract_value_from_default also
    needs to handle escaped strings.
  * Both PostgreSQL and CockroachDB use C-style string escapes under the covers.
    PostgreSQL obscures this for us and unescapes the strings, but CockroachDB
    does not. extract_value_from_default can unescape the strings via Ruby.
  * The PostgresqlDefaultExpressionTest tests fail because CockroachDB uses
    different functions than PostgreSQL for current date/timestamps.
  * The test has been replaced by CockroachDB::DefaultExpressionTest, and it
    uses CockroachDB's current_date() and current_timestamp() functions in its
    assertions.
@alimi
Copy link
Contributor Author

alimi commented Apr 15, 2020

I added a couple more changes to fix things I came across in test/cases/defaults_test.rb.

7ad6f53 updates extract_value_from_default so it handles escaped strings. See cockroachdb/cockroach#47497.

98bf1a8 replaces an AR test to work with CockroachDB's current_date() and current_timestamp() functions.

Copy link
Contributor

@apantel apantel left a comment

Choose a reason for hiding this comment

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

LGTM

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

2 participants