Skip to content

Travis: also test on MyISAM #78

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

Closed
wants to merge 3 commits into from

Conversation

julen
Copy link
Contributor

@julen julen commented Apr 10, 2014

Haven't had a chance to actually try this, let's hear from Travis-CI itself...

Also: I'm not sure what MySQL version Travis-CI runs on, it only mentions 5.5.x, but the fact is that InnoDB became the default only in 5.5.5.

@julen
Copy link
Contributor Author

julen commented Apr 11, 2014

The build failures are related to:

  • The MySQL connector (MySQL-python) not supporting Python 3.2/3.3. The alternative looks like using PyMySQL. I can change this in the .travis.yml file.
  • Django 1.7 support (basically fixing use a test runner that is declared in settings #66, or at the very least trying to use django.test.runner.DiscoverRunner first before falling back to django.test.simple.DjangoTestSuiteRunner)

@julen
Copy link
Contributor Author

julen commented Apr 11, 2014

I talked too fast about PyMySQL, since it's not compatible with Django these days. I'll restore the excludes.

@pelme
Copy link
Member

pelme commented Apr 13, 2014

Thanks for tackling this.

Can you please update this PR to the latest master, it should probably take care of all the failures seen on Travis.

@julen
Copy link
Contributor Author

julen commented Apr 13, 2014

There you go.

@pelme
Copy link
Member

pelme commented Apr 14, 2014

Thanks! It looks like the exclusion rules will need to be updated too, to exclude Django master+Python 2.6.

@julen
Copy link
Contributor Author

julen commented Apr 14, 2014

I just did it, actually — OTOH it's worth noting that master is 1.8 already, but since 1.7 is not available on PyPi yet it's now untested.

@pelme
Copy link
Member

pelme commented Apr 14, 2014

Thanks, I was very confused for a minute when I saw a new build on Travis at the same time as my comment. :)

Yes, we should add the 1.7 beta tar ball until 1.7 hits PyPI. The build matrix is getting huge and a bit tedious to manage, but I think it should work.

@pelme
Copy link
Member

pelme commented Apr 14, 2014

It is strange that the build does not fail on MyISAM though, are you sure the init_command option is not needed? The pytest_django and pytest_django_reuse database are mostly created to workaround https://code.djangoproject.com/ticket/16969

@julen
Copy link
Contributor Author

julen commented Apr 14, 2014

No, I'm not sure :) TBH at this stage I ignore how the DB initialization process works.

I can adapt the patch to use separate settings files for InnoDB and MyISAM and issue the init_command from there, passing the relevant SET STORAGE_ENGINE=<XYZ> there. How does that sound?

@pelme
Copy link
Member

pelme commented Apr 14, 2014

That sounds like a good idea!

@julen
Copy link
Contributor Author

julen commented Apr 14, 2014

Done, hopefully this makes more sense. Also, you could maybe accelerate getting the results by cancelling old/obsolete test runs.

@pelme
Copy link
Member

pelme commented Apr 14, 2014

Thanks, I cancelled build 225 now.

@pelme
Copy link
Member

pelme commented Apr 14, 2014

Now there are some failures on MyISAM. :)

I think it would be good to have all MyISAM changes in this branch, so that we can ensure it all works before merging to master.

It looks like some other tests that deals with transactions needs to be skipped on MyISAM too.

See https://bitbucket.org/hpk42/pytest/src/17a246750c75f5eda01d065b9c72ab9fb0f1b544/_pytest/python.py?at=default#cl-1756

pytest's addfinalizer method appends finalizers to a list, and pops them off the end to call in finish(). These two lines must be switched to prevent _post_teardown accessing the blocking CursorWrapper.

It's a bit ugly in this case, but you might want to use yield_fixtures to make the ordering more explicit:

```
@pytest.yield_fixture
def django_db(_django_cursor_wrapper):
    if ('transactional_db' not in request.funcargnames and
            'live_server' not in request.funcargnames and
            not is_django_unittest(request.node)):

        from django.test import TestCase

        with _django_cursor_wrapper:
            case = TestCase(methodName='__init__')
            case._pre_setup()
            yield
            case._post_teardown()

    yield
```

It looks pretty, except for that yield at the end, which is necessary when that if case is False.
@julen
Copy link
Contributor Author

julen commented Apr 14, 2014

Cherry-picked changes from #64.

I'm not sure how to tell @pytest.mark.skipif() that MyISAM is being used without coupling the tests to be skipped to the Travis setup. Any ideas?

@pelme
Copy link
Member

pelme commented Apr 14, 2014

There is a "connections_support_transactions" that can be used.
https://github.com/django/django/blob/master/django/test/testcases.py#L850

Something like

from django.test.testcases import connections_support_transactions

def test_that_needs_transactions():
    if not connections_support_transactions():
        pytest.skip('transactions required for this test')

    # ...

could work.

Thanks to Andreas Pelme for the suggestion.
@julen
Copy link
Contributor Author

julen commented Apr 14, 2014

Thanks! Locally works fine; let's see what Travis says...

@pelme
Copy link
Member

pelme commented Apr 14, 2014

Thanks a lot for putting in the effort to fix this! I have merged all the changes, and Travis is building everything in the master branch now!

@pelme pelme closed this Apr 14, 2014
@julen julen deleted the feature/travis-myisam branch April 14, 2014 19:39
@julen
Copy link
Contributor Author

julen commented Apr 14, 2014

Thanks to you for keeping pytest-django alive!

Just wondering: any blockers for a next release on PyPI? Maybe you want to ensure Django 1.7 support before that happens?

@pelme
Copy link
Member

pelme commented Apr 14, 2014

No blockers. I am just going crazy of the explosion of combinations in Travis. I want to make sure Django 1.7 and Python 3.4 are tested+fully supported before the release.

I making a script to generate tox.ini+.travis.yml for all valid, supported versions. I also want to be able to run things locally since Travis takes ages.

Anyways, I hope to get a new version on PyPI during the week.

@julen
Copy link
Contributor Author

julen commented Apr 14, 2014

That sounds great, and thanks for the effort!

adamchainz added a commit to adamchainz/pytest-django that referenced this pull request Mar 6, 2025
MyISAM testing was added back in 2012 (pytest-dev#78) because it would have detected one bug in relation to transactional ordering.

MyISAM was not recommended back then and even less so now. It’s non-transactional nature has many implications and [Django’s database documentation](https://docs.djangoproject.com/en/stable/ref/databases/) caveats against using it.

I think we can remove the testing now and reduce the maintenance burden. As far as I can tell, it was only added retroactively to guard against that one bug, 13 years ago.
adamchainz added a commit to adamchainz/pytest-django that referenced this pull request Mar 6, 2025
MyISAM testing was added back in 2012 (pytest-dev#78) because it would have detected one bug in relation to transactional ordering.

MyISAM was not recommended back then and even less so now. It’s non-transactional nature has many implications and [Django’s database documentation](https://docs.djangoproject.com/en/stable/ref/databases/) caveats against using it.

I think we can remove the testing now and reduce the maintenance burden. As far as I can tell, it was only added retroactively to guard against that one bug, 13 years ago.
adamchainz added a commit that referenced this pull request Mar 6, 2025
MyISAM testing was added back in 2012 (#78) because it would have detected one bug in relation to transactional ordering.

MyISAM was not recommended back then and even less so now. It’s non-transactional nature has many implications and [Django’s database documentation](https://docs.djangoproject.com/en/stable/ref/databases/) caveats against using it.

I think we can remove the testing now and reduce the maintenance burden. As far as I can tell, it was only added retroactively to guard against that one bug, 13 years ago.
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.

3 participants