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

Add .travis.yml #28

Merged
merged 5 commits into from
Mar 15, 2017
Merged

Add .travis.yml #28

merged 5 commits into from
Mar 15, 2017

Conversation

aried3r
Copy link
Contributor

@aried3r aried3r commented Mar 14, 2017

This probably won't work out of the box, but should be a good starting point.

Gemfiles/Ruby combinations seem to work:
screen shot 2017-03-14 at 12 35 23

@aried3r
Copy link
Contributor Author

aried3r commented Mar 14, 2017

The tests now run via PostgreSQL but they fail. It seems many tests are run with database models although they are only added for SQLite?

@aried3r
Copy link
Contributor Author

aried3r commented Mar 14, 2017

Changed it to SQLite, in case the tests then work.

@janxious
Copy link
Contributor

Basically there are different tests for the transactional systems under test. You can see that at work in test/transaction_test.rb. It may not be necessary to differentiate any more. This project was created before sqlite had transactions and I think it's possibly a relic of that.

Said another way, we could probably strip out all the database-specific pieces now because they were valuable 4 or 5y ago but now are probably just getting in the way.

@aried3r
Copy link
Contributor Author

aried3r commented Mar 14, 2017

I think I get what you're saying, but I don't yet quite grasp what is going on in transaction_test.rb. Do you think you could help me and apply the necessary changed in this branch? Maybe we can get a successful SQLite run after your changes.

I guess the PgArchival and MysqlArchival classes will go, which would make #29 obsolete.

@janxious
Copy link
Contributor

janxious commented Mar 14, 2017

I don't yet quite grasp what is going on in transaction_test.rb

It basically is the test that will connect to mysql/postgres to make sure data is not being persisted in the case of exception during archive/unarchive. I don't think we need to be specific any more as regards the db engine. For the purposes of this gem's functioning, mysql, postgres, and sqlite have feature parity around transactions because we are not using checkpoints or sub-transactions.

So… yeah, we can probably just nuke anything that deals with mysql or postgres, including all the setup junk.

@janxious
Copy link
Contributor

ANd yeah, #29 is not necessary if we no longer connect to mysql or pg at all. :D

@janxious
Copy link
Contributor

I tried a bunch of stuff to get the mysql bits to run correctly. There is apparently a big snafu around mysql on travis you can see at travis-ci/travis-ci#6842

I think removing mysql and postgresql will probably be best, despite making it slightly harder to write tests for regressions in the future, should they arise.

@aried3r
Copy link
Contributor Author

aried3r commented Mar 15, 2017

I ran into the same issues with MySQL on Travis.

I removed all MySQL and PostgreSQL code and am starting to see the first successful test runs on Travis. I'll have a look what makes the other ones fail.

@aried3r
Copy link
Contributor Author

aried3r commented Mar 15, 2017

A green test run!

How do you like the approach so far?

@aried3r
Copy link
Contributor Author

aried3r commented Mar 15, 2017

Btw, I'd love to add the Rails 5.1 beta, but allowing it to fail if you're ok with that.

Other TODOs include changing the README and adapting the CHANGELOG.

Also, I'd love to add RuboCop with a few selected rules, like forcing Ruby 1.9 hash syntax etc. But in another PR.

@@ -40,7 +40,7 @@ def setup_database_cleaner

def create_test_tables
schema_file = File.dirname(__FILE__) + "/schema.rb"
["pg", "mysql", "sqlite"].each do |db|
["sqlite"].each do |db|
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably doesn't need to be inside an each block any more.

@janxious
Copy link
Contributor

How do you like the approach so far?

Seems correct. There are some instances of sqlite being mentioned specifically (in at least the transaction test), which is unneccessary.

Btw, I'd love to add the Rails 5.1 beta, but allowing it to fail if you're ok with that.

👌 sounds fine

Other TODOs include changing the README and adapting the CHANGELOG.

Not sure what you mean about the changelog. It probably doesn't need to change at all until a new version is cut. Even then there shouldn't be any changes to old entries aside from grammatical errors. Is that what you have in mind, or were you thinking something else?

• • •

What is up with this commit? c4f33ee 😁

@aried3r
Copy link
Contributor Author

aried3r commented Mar 15, 2017

Seems correct. There are some instances of sqlite being mentioned specifically (in at least the transaction test), which is unneccessary.

Done.

Not sure what you mean about the changelog. It probably doesn't need to change at all until a new version is cut.

You're right! I guess only the README has to be adapted that this gem is no longer being tested explicitly on PostgreSQL and MySQL?

What is up with this commit? c4f33ee 😁

Heh, I think it's because Ruby 2.4 has unified Fixnum and Binum into Integer which arel 5 (which I guess is what shipped with Rails 4.1) doesn't support. You can see a failed test run here.

@aried3r
Copy link
Contributor Author

aried3r commented Mar 15, 2017

Hmm, I'm not sure why Travis suddenly can't find Ruby 2.4 anymore in some cases. That's the one thing that used to work, haha.

@aried3r
Copy link
Contributor Author

aried3r commented Mar 15, 2017

I found travis-ci/travis-ci#7077 and will add the minor versions as well to see if this clears things up.

Btw, sorry for the many messages, but one more thing. Do you want me to clean up the commits (squash a few of them together) or are you just gonna squash them all into one using GitHub's Squash+Merge?

@janxious
Copy link
Contributor

Do you want me to clean up the commits (squash a few of them together) or are you just gonna squash them all into one using GitHub's Squash+Merge?

If you prefer that. It's your PR. I don't normally squash because I prefer the full history.

Just let me know when you are done. I think this is 👍

@aried3r
Copy link
Contributor Author

aried3r commented Mar 15, 2017

I'm done. I'm just waiting for another successful test run and it's ready to be merged from my point of view. Thanks for all the support!

@janxious janxious merged commit bd5a9e8 into expectedbehavior:master Mar 15, 2017
@janxious
Copy link
Contributor

👍 you are very welcome. Thanks for getting this green!

I added you to the contributor list. If you would like to not be there, let me know.

@janxious
Copy link
Contributor

@aried3r

It probably doesn't need to change at all until a new version is cut

Released 1.2.0 today. Thanks again for all the help and the push to clean things up!

@aried3r
Copy link
Contributor Author

aried3r commented Mar 19, 2017 via email

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.

2 participants