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

Clean up top level directory of repository #3738

Merged
merged 1 commit into from
Apr 4, 2020

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Jan 31, 2020

fix #3730

@ltalirz ltalirz force-pushed the issue_3730_cleanup_toplevel branch 2 times, most recently from 31339d9 to 1580ce1 Compare April 1, 2020 10:04
@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #3738 into develop will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3738      +/-   ##
===========================================
- Coverage    78.01%   78.00%   -0.01%     
===========================================
  Files          457      457              
  Lines        33830    33830              
===========================================
- Hits         26391    26390       -1     
- Misses        7439     7440       +1     
Flag Coverage Δ
#django 70.05% <ø> (ø)
#sqlalchemy 70.87% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
aiida/transports/plugins/local.py 80.20% <0.00%> (-0.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f40f0d2...1c369a3. Read the comment docs.

@ltalirz ltalirz requested a review from sphuber April 1, 2020 11:15
@ltalirz
Copy link
Member Author

ltalirz commented Apr 1, 2020

@sphuber despite the current issues with CI, I believe the tests actually pass (see 2nd-to-last commit) and the coverage should not be decreasing.

@sphuber
Copy link
Contributor

sphuber commented Apr 1, 2020

The tests are failing because it is still trying to install the bin/runaiida script during package setup:

error: [Errno 2] No such file or directory: '/home/runner/work/aiida-core/aiida-core/bin/runaiida'

You should remove the scripts key from the setup.json as well. Then we should really test that in a completely new environment the #!/usr/bin/env runaiida shebang still works exactly the same

@ltalirz ltalirz force-pushed the issue_3730_cleanup_toplevel branch from 950df60 to 091c4b0 Compare April 1, 2020 12:06
@ltalirz
Copy link
Member Author

ltalirz commented Apr 1, 2020

The tests are failing because it is still trying to install the bin/runaiida script during package setup:

Ah... ;-) Thanks, fixed!

Then we should really test that in a completely new environment the #!/usr/bin/env runaiida shebang still works exactly the same

I checked it on my side and it worked. Please check as well.

@ltalirz ltalirz force-pushed the issue_3730_cleanup_toplevel branch from 091c4b0 to 6bbeeff Compare April 1, 2020 12:38
@sphuber
Copy link
Contributor

sphuber commented Apr 1, 2020

@ltalirz I also verified that the new runaiida still works. The pre-commit is now failing. These are the lines that you addressed in another PR, that we were wondering how they came there. I guess you must have mixed those files from this PR into the other one. Anyway, should be an easy fix. Once that is done I will merge this. Thanks!

@ltalirz
Copy link
Member Author

ltalirz commented Apr 1, 2020

These are the lines that you addressed in another PR, that we were wondering how they came there.

Indeed, this is what I introduced here because the tests suddenly started failing.
I don't see these changes anymore in the final squashed commit of yours in #3875

Very strange... how is it possible that the tests did pass for you while they did not for me?

Anyhow, I will re-introduce the changes here.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 1, 2020

Just to follow up on this, here are links to recent github actions runs:

  • passed (despite the # pylint:disable comments missing)
  • failed

Looking at the python environment, the only differences are:

7c7
< Werkzeug-1.0.0
---
> Werkzeug-1.0.1
42c42
< django-2.2.11
---
> django-2.2.12
84c84
< nbformat-5.0.4
---
> nbformat-5.0.5

I don't think those have anything to do with it (also, if the reason for this would be updates of python dependencies, it would be unclear why the pre-commit hooks worked, then broke, then worked again, then broke again).

@sphuber
Copy link
Contributor

sphuber commented Apr 1, 2020

The tests are failing for SqlAlchemy. They seem to be hanging at tests/backends/aiida_sqlalchemy/test_generic.py. I have already restarted them once but they timed out at the same point. I am not sure what is going on

@ltalirz
Copy link
Member Author

ltalirz commented Apr 2, 2020

I can reproduce the issue locally using

$ AIIDA_TEST_BACKEND=sqlalchemy pytest tests/backends/aiida_sqlalchemy

Actually, the tests in test_generic.py run fine for me:

 $ AIIDA_TEST_BACKEND=sqlalchemy pytest tests/backends/aiida_sqlalchemy/test_generic.py
============================================================================================================ test session starts =============================================================================================================
platform darwin -- Python 3.7.1, pytest-5.3.4, py-1.8.0, pluggy-0.13.1
rootdir: /Users/leopold/Personal/Postdoc-MARVEL/repos/aiida/aiida_rmq, inifile: pytest.ini
plugins: timeout-1.3.4, datadir-1.3.1, cov-2.8.1
collected 4 items

tests/backends/aiida_sqlalchemy/test_generic.py ....                                                                                                                                                                                   [100%]

============================================================================================================= 4 passed in 8.17s ==============================================================================================================

For me it's hanging at tests/backends/aiida_sqlalchemy/test_migrations.py

$ AIIDA_TEST_BACKEND=sqlalchemy pytest tests/backends/aiida_sqlalchemy
============================================================================================================ test session starts =============================================================================================================
platform darwin -- Python 3.7.1, pytest-5.3.4, py-1.8.0, pluggy-0.13.1
rootdir: /Users/leopold/Personal/Postdoc-MARVEL/repos/aiida/aiida_rmq, inifile: pytest.ini
plugins: timeout-1.3.4, datadir-1.3.1, cov-2.8.1
collected 39 items

tests/backends/aiida_sqlalchemy/test_generic.py ....                                                                                                                                                                                   [ 10%]
tests/backends/aiida_sqlalchemy/test_migrations.py ^C

@ltalirz
Copy link
Member Author

ltalirz commented Apr 2, 2020

It actually hangs here:

self.migrate_db_down(self.migrate_from)

Is it a problem if a user is already stored in the DB at this point?
How do the migration tests work?

Perhaps @giovannipizzi could also comment

P.S. Fast way to reproduce:

AIIDA_TEST_BACKEND=sqlalchemy pytest tests/backends/aiida_sqlalchemy/test_migrations.py::TestMigrationEngine::test_revision_numbers

@giovannipizzi
Copy link
Member

giovannipizzi commented Apr 2, 2020

I might be fixing this in #3650

[EDIT: for reference, the problem that I am fixing is that the downward migration test was not resetting the migration to the most recent one, and the next one to run, if any, would fail; however it would not hang for me. I suggest to merge #3650 first and see if this solves the issue already]

@ltalirz
Copy link
Member Author

ltalirz commented Apr 4, 2020

Yeah, I introduced this fixture because it was necessary.

There are two options for this PR - either we fix the underlying issue or we keep the "pytest tests" separate from the rest (it's not strictly about cleaning up the top-level of the directory).

Note, however, that we will likely run into this issue again soon, namely when people start to use pytest fixtures in the test suite more actively.

@sphuber
Copy link
Contributor

sphuber commented Apr 4, 2020

I am just playing around with this now and it indeed seems a problem in the two different ways be currently setup and clean the database. By adding the fixture, we ensure that a user instance is created for the pytest tests, however, this interferes with the setup of the AiidaTestCase tests and particularly with the migrations. Given that already the moving is really a bit outside of the scope of this PR, I would vote to remove it for now and continue in a separate issue.

@ltalirz
Copy link
Member Author

ltalirz commented Apr 4, 2020

Fine with me - can you take care of it?

@sphuber sphuber force-pushed the issue_3730_cleanup_toplevel branch from 6a9c382 to 91be0c6 Compare April 4, 2020 20:00
@sphuber
Copy link
Contributor

sphuber commented Apr 4, 2020

Fine with me - can you take care of it?

done

@ltalirz
Copy link
Member Author

ltalirz commented Apr 4, 2020

Looks good to me - you'll need to approve yourself ;-)

 * Include `bin/runaiida` through `console_scripts` of `setup.json`
 * Remove the outdated examples from `examples` directory
 * Remove obsolete `utils/plugin_tpl/calculation.tpl` superseded by
   plugin cookie cutter package
 * Move `conftest` to the `tests` directory
@sphuber sphuber force-pushed the issue_3730_cleanup_toplevel branch from 91be0c6 to 1c369a3 Compare April 4, 2020 20:19
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Wij van WC-eend adviseren WC-eend 👍

@sphuber sphuber merged commit 5c0d86f into aiidateam:develop Apr 4, 2020
@sphuber sphuber deleted the issue_3730_cleanup_toplevel branch April 4, 2020 20:37
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.

cleanup of top-level dir
3 participants