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.

Sign up for GitHub

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

Jump to bottom

Add travis config for testing against multiple databases #236

Merged
merged 2 commits into from
Oct 25, 2016
Merged

Add travis config for testing against multiple databases #236

merged 2 commits into from
Oct 25, 2016

Conversation

fschwahn
Copy link
Contributor

@fschwahn fschwahn commented Oct 21, 2016 •
edited
Loading

As discussed in #76, it's a good idea to test against the most commonly used databases (ie. mysql, postgres, sqlite). This pull request adds support for that. Some notes:

  • Added database.yml with credentials. These credentials are the default from travis ci (for mysql, postgres). If no database is configured it still defaults to sqlite, meaning for existing developers nothing should change.
  • Some tests were failing in mysql / postgres because they tested database specific behaviour (mostly order of nulls / default order). I did the minimum amount of work to get things passing, which in some cases meant flat out removing assertions.
  • I added memoization for now and yesterday (see diff), because the test could fail in slow environments (like travis).
  • I added an allow_fail directive for rbx-2, because it won't install on travis right now. capistrano did the same things recently because rbx support is so flaky.
  • I added a cache for bundler which should speed up builds (I saw devise doing this and thought it couldn't hurt).

@fschwahn fschwahn force-pushed the travis_multi_database branch from 318259a to d31cad4 Compare October 21, 2016 08:36
brendon
brendon requested changes Oct 22, 2016
View reviewed changes
Copy link
Owner

@brendon brendon 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 great work @fschwahn! :) I just had the one comment to look at (very minor). I'm ok with removing those other tests as we're kind of double testing there anyway.

A great step forward :)

test/test_list.rb
@@ -768,13 +759,13 @@ def test_nil_position_ordering
new3.reload.pos = 1
new3.save

assert_equal [nil, 1, 2], DefaultScopedMixin.all.map(&:pos)
assert_equal [2, 3, 1], DefaultScopedMixin.all.map(&:id)
assert_equal [1, 2], DefaultScopedMixin.where("pos IS NOT NULL").map(&:pos)
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps we should be checking that new2 has a nil position still to keep with the spirit of the original test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense - I added the assertion

@brendon brendon merged commit 6bec97e into brendon:master Oct 25, 2016
@brendon
Copy link
Owner

brendon commented Oct 25, 2016

Thanks @fschwahn :)

@brendon
Copy link
Owner

brendon commented Oct 25, 2016

Just noticed this failure: https://travis-ci.org/swanandp/acts_as_list/jobs/170359139 and one other on that test run. Would you mind investigating timecop as an alternative for freezing the time for comparison?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendon brendon brendon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fschwahn @brendon