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

Prevent tests from deleting the default user #2480

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Feb 14, 2019

Fix #2407

PR #2214 (inadvertently?) removed safeguards that prevented the default user from
being deleted in between tests.
Since AiiDA expects a default user in many operations, leaving the DB
in a state with no default user is not ideal.

This PR reintroduces safeguards that result in the default user
remaining untouched.

  • reintroduce safeguards for django
  • introduce safeguards for sqlalchemy
  • add testimplbase to pre-commit

PR aiidateam#2214 stripped of safeguards that prevented the default user from
being deleted in between tests.
Since AiiDA expects a default user in many operations, leaving the DB
in a state with no default user is not ideal.

This commit reintroduces safeguards that result in the default user
remaining untouched.
@ltalirz ltalirz changed the title Issue 2407 tests delete default user Prevent tests from deleting the default user Feb 14, 2019
@giovannipizzi
Copy link
Member

I've approved, but actually a few tests fail.
Also, I realised that the Jenkins machine was in a weird state with problems with docker, I upgraded it and rebooted, let's see if also the Jenkins tests start to work again.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.1%) to 63.609% when pulling edec57f on ltalirz:issue_2407_tests_delete_default_user into b1b34f6 on aiidateam:provenance_redesign.

@ltalirz
Copy link
Member Author

ltalirz commented Feb 14, 2019

This did not resolve the issue observed in #2400.
After discussion with @szoupanos , the behavior observed in #2407 may actually be acceptable.
Will close #2407 once the problem behind #2400 has been identified.

@szoupanos
Copy link
Contributor

Just a small comment on this:

I just wanted to say that since we decided to expect a clean database at the beginning of each test class and leave a clean database at the end of each test class, we should ensure that this happens in both backends.

Maybe what I say it is obvious and it is already done, but I wanted to mention it.

@sphuber
Copy link
Contributor

sphuber commented Mar 1, 2019

I would tend to agree that we should be able to delete all resources, including default users for tests databases, and if they are needed for certain operations it should be recreated. The problem you experienced during #2400 has another root cause I think, where as you said, certain classes in AiiDA can retain references to ORM instances that point to database model instances that have been cleaned, leading to problems mostly in SqlAlchemy. You already opened an issue in #2515 to address a (part) of this question, so I suggest we try to solve the greater problem instead.

@ltalirz ltalirz closed this Mar 1, 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.

5 participants