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

Enable testing with MySQL and Postgres on Travis-CI #171

Closed
wants to merge 6 commits into from

Conversation

atodorov
Copy link
Contributor

This PR enables MySQL and Postgres on Travis. I have a support ticket opened with their team to figure out how to add MariaDB alongside MySQL.

The build will fail b/c there are some issue with both MySQL and Postgres. I will try to fix them in subsequent commits.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling c6c2794 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling f486a3d on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling 297333c on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling b9e8ab6 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling b9e8ab6 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling cefa25a on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling 6f9fdcd on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling e34804a on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.189% when pulling 1c2d2fd on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling bad6c4c on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling bad6c4c on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 482f977 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 482f977 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

coveralls commented Mar 12, 2017

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 482f977 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 443c5bb on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 443c5bb on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling a5920d7 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 414963e on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 288c9fc on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 288c9fc on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.03%) to 45.209% when pulling 11b4fb9 on MrSenko:travisci_more_db_testing into ae3745a on Nitrate:develop.

@atodorov
Copy link
Contributor Author

@tkdchen I think this is ready to merge so we can continue forward. I hate to merge with failing Postgres but MySQL, MariaDB and sqlite all work fine and some of the fixes are covered into other PRs. I don't think we have a lot of other options here otherwise this PR will become very big and there are other problems to worry about too.

@tkdchen
Copy link
Member

tkdchen commented Apr 12, 2017

This is a big step forward to the testing. However, the problem is there is dependency of Travis in Nitrate source code, and this testing matrix cannot run locally by developer. I prefer to a solution to avoid these two major disadvantages.

Basically, the testing matrix should

  • allow developer to run tests locally without any other external services required.
  • allow developer to run tests by selecting specific database, SQLite, MySQL, MariaDB or PostgreSQL, or even all of them at once whatever in parallel or in sequence.
  • by default, tests could be run with SQLite, which is a much fast way to run tests and verify something quickly.

The second ability should be able to be used in Travis-CI to run the testing matrix.

In my mind, docker would be a good tool to help us to build this testing matrix, and obviously some scripts would be required.

Currently, to run whole test suite, I can simple run make test in a well-provisioned vagrant machine. However, running test suite from scratch should be an easy thing now in Nitrate as following steps,

  1. provision machine, installing necessary packages required in next steps
  2. make a virtual environment
  3. install dependencies from requirements/devel.txt
  4. make test

It should be easy to make these steps happen in a docker container. Fedora 25 docker image can be used and it is ok to use the version of MySQL, MariaDB and PostgreSQL in Fedora repo.

Finally, there could be these ways to run tests matrix

  • make test, run with SQLite by default
  • DB=[dbname] make test, dbname could be mysql, mariadb, or pgsql
  • DB=all make test, run tests with mysql, mariadb, and pgsql individually

Thank you for making this improvement. It's valuable and develop our ideas. Let's make it better and better.

I'll look at other commits in this PR from tomorrow. Have a good day. :)

@atodorov
Copy link
Contributor Author

make test, run with SQLite by default
DB=[dbname] make test, dbname could be mysql, mariadb, or pgsql
DB=all make test, run tests with mysql, mariadb, and pgsql individually

I think this can easily be done in the settings and then simply ask Travis to execute whichever command fits its matrix. In this way we move the infrastructure dependency in .travis.yml and not in the project source code.

I will rework the settings for that. It is up to the developer to prepare their local environment for whichever DB they want to test on. I will also update the docs with commands how to install dependencies and run the tests against different databases.

Note: vagrant or docker will be a great help but that's up to each developer to decide how to use. IMO this needs to come at a later stage because this PR is quite big already. What do you think?

@atodorov atodorov force-pushed the travisci_more_db_testing branch 2 times, most recently from 4888d77 to bd74f39 Compare April 13, 2017 13:38
@coveralls
Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage increased (+0.05%) to 45.425% when pulling bd74f39 on MrSenko:travisci_more_db_testing into 8f45beb on Nitrate:develop.

@atodorov
Copy link
Contributor Author

@tkdchen see the updated documentation and the last 3 commits for the latest changes. It is not possible to execute the tests on the local machine by specifying which database to use. The same commands are integrated into Travis as well. All of the integration happens in .travis.yml.

@tkdchen
Copy link
Member

tkdchen commented Apr 14, 2017

Using Travis to run tests does not mean there is a dependency of Travis to run tests. Simply say I don't want this line of code appears in source code

if 'TRAVIS' in os.environ:

Documentation is good but not always. For running tests, I prefer to make it as easy as possible with just one command, no need of steps

  1. find the documentation
  2. read through the documentation to learn the steps
  3. try each step to provision environment and launch tests
  4. repeat again and again if something is wrong by misoperation

Instead, as mentioned in my previous comment, just make test or DB=pgsql make test is enough. Easy and save much time, isn't it?

If someone is interested in the steps to build testing environment, just open and read the relative scripts. That should be more straightforward than lots of words in documentation and easy understand.

@coveralls
Copy link

coveralls commented Apr 14, 2017

Coverage Status

Coverage increased (+0.03%) to 45.411% when pulling 61355c2 on MrSenko:travisci_more_db_testing into 8f45beb on Nitrate:develop.

@atodorov
Copy link
Contributor Author

if 'TRAVIS' in os.environ:

Mistake, this has now been updated. The rest (make test commands) are as requested and described in more details in the docs. Anything else missing ?

@coveralls
Copy link

coveralls commented Apr 14, 2017

Coverage Status

Coverage increased (+0.03%) to 45.406% when pulling 8b42d6f on MrSenko:travisci_more_db_testing into 8f45beb on Nitrate:develop.

@tkdchen
Copy link
Member

tkdchen commented Apr 15, 2017

@atodorov We talked a lot in IRC channel #nitrate. You did a good job to make it possible to run tests on different database engines. Hopefully, you could continue improving based on the changes in this PR to make running tests more easily.

I'd like to repeat my thought again

  1. Do not depend on Travis-CI to run tests.
  2. To achieve the first goal, if those database provision steps in .travis.yml are necessary, they should happen locally, not in Travis-CI.
  3. As a developer, I can run tests simply in following steps without the need of reading whole documentation before that.
    1. clone Nitrate
    2. DB=mysql make test

Detailed documentation in this PR is good, I think it could be as reference for developers who are interested in hacking the tests for themselves.

Is this clear and make sense?

@tkdchen
Copy link
Member

tkdchen commented Apr 15, 2017

I merged these 3 commits

a1c47ec Compare PKs with what was written in DB, not hard-coded
fc2343c Update pre_check_product() and pre_process_ids() to include long
7d10b49 Don't use unique test object names

@@ -37,7 +36,7 @@ def setUpTestData(cls):
cls.client = test.Client()

def test_404_if_run_does_not_exist(self):
nonexisting_run_pk = TestRun.objects.count() + 1
nonexisting_run_pk = 999999
Copy link
Member

Choose a reason for hiding this comment

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

It would be helpful to understand if you can add explanation to this change. Then, I'll merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, for some reason ( I think because PKs are global autoincrements on MySQL, generated via LAST_INSERT_ID())

TestRun.objects.count() == 1
nonexisting_run_pk == 2

but the only TestRun object in the database has an actual ID of 2 hence the failure!

On other DB engines the only TestRun object has an ID of 1. I will update this code with
TestRun.objects.last().pk + 1 which will be more robust.

However I have found one more thing. In testruns/urls/run_urls.py there are two URLs which use the same view:
.../ordercase/$ and .../ordercaserun/$. Do you know why is that? Sounds like a bug (let me know and I will send a separate PR for it).

Fix Nitrate#169

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
- the older Travis CI images have problems with MariaDB not picking
  up its configuration
- also manually create DB and test DB and grant privileges to travis
  b/c connection will fail otherwise.

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
On MySQL the default AutoField() translates to AUTO_INCREMENT
which is shared between all tables!

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
testing is now possible via the following commands:

	make test (uses SQlite)
	TEST_DB=all make test (will test on all DBs)
	TEST_DB=MySQL make test
	TEST_DB=MariaDB make test
	TEST_DB=Postgres make test

In addition when preparing the local virtualenv we now have DB
specific requirements files:

	requirements/mysql.txt (for MySQL and MariaDB)
	requirements/postgres.txt (for Postgres)
	requirements/base.txt (for SQLite)
	requirements/devel.txt (additional development libraries)

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
also updates the usage of new requirements files

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@atodorov
Copy link
Contributor Author

Updated and rebased.

@tkdchen let's move this comment #171 (comment) into a separate issue. I understand the value proposition of it and it is definitely doable even in the short term. However not everyone would like to follow exactly the same steps so we shouldn't force it. Plus a separate issue will allow us to discuss into more details. What do you think?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 45.406% when pulling fa98cb2 on MrSenko:travisci_more_db_testing into a1c47ec on Nitrate:develop.

1 similar comment
@coveralls
Copy link

coveralls commented Apr 15, 2017

Coverage Status

Coverage increased (+0.04%) to 45.406% when pulling fa98cb2 on MrSenko:travisci_more_db_testing into a1c47ec on Nitrate:develop.

@atodorov atodorov mentioned this pull request Apr 26, 2017
@tkdchen
Copy link
Member

tkdchen commented Feb 4, 2019

This was done in 2e22b8a and subsequent commits that make it possible to run with PostgreSQL.

@tkdchen tkdchen closed this Feb 4, 2019
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

3 participants