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

Sdem/improving tests #7246

Merged
merged 15 commits into from
Jul 17, 2020
Merged

Sdem/improving tests #7246

merged 15 commits into from
Jul 17, 2020

Conversation

GuillermoCasas
Copy link
Member

@GuillermoCasas GuillermoCasas commented Jul 15, 2020

Description
I made some updates to the tests with the intention of adding them to the automatic nightly runs. Please, @philbucher , can you take a look and tell me if it is possible?

Changelog

  • Removing non-standard python imports
  • Making the structure of the tests a bit tidier
  • Removing some deprecation warnings (there remains one but its solution is left for later)

Additional info
It will be very positive to have the tests there, since the application keeps breaking from time to time due to the sporadic use.

@GuillermoCasas GuillermoCasas added Testing Continuous Integration related to Travis, Appveyor, ... labels Jul 15, 2020
@GuillermoCasas GuillermoCasas requested a review from a team as a code owner July 16, 2020 09:39
Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

overall it looks good to me, I added the app to the CI to check if it works (note that this will require final approval from the technical committee, but it should only be a formality here)

I saw that you are using

kratos_add_dependency(${KRATOS_SOURCE_DIR}/applications/FluidDynamicsApplication)
kratos_add_dependency(${KRATOS_SOURCE_DIR}/applications/DEMApplication)

I recommend importing these apps in your SwimmingDEMApplication.py to avoid problems related to imports. This is e.g. how er do it in the TrilinosApp, which depends on the MPICore

@@ -1,6 +1,8 @@
from __future__ import print_function, absolute_import, division # makes KratosMultiphysics backward compatible with python 2.6 and 2.7
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from __future__ import print_function, absolute_import, division # makes KratosMultiphysics backward compatible with python 2.6 and 2.7

can be removed now, just a comment

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so, but I have not seen it removed in most places. Perhaps this could be done all at once.

@@ -1,10 +1,7 @@
import math
import cmath
import mpmath
import scipy.special
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

neither scipy nor numpy are part of the python standard library, hence if you use this in a test and they are not available, the test will fail

Check this or this to see how I solve those problems, but it can be done in different ways too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can do that.

Copy link
Member

Choose a reason for hiding this comment

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

this is how it looks like otherwise (output of the Win CI)

Running SwimmingDEMApplication tests
Traceback (most recent call last):
  File "d:\a\Kratos\Kratos\bin\Custom\applications//SwimmingDEMApplication/tests/test_SwimmingDEMApplication.py", line 11, in <module>
    import NightTests
  File "d:\a\Kratos\Kratos\bin\Custom\applications\SwimmingDEMApplication\tests\NightTests.py", line 10, in <module>
    import TestFactory as TF
  File "d:\a\Kratos\Kratos\bin\Custom\applications\SwimmingDEMApplication\tests\TestFactory.py", line 9, in <module>
    from tests_python_scripts.candelier_scripts.candelier_analysis import CandelierBenchmarkAnalysis
  File "d:\a\Kratos\Kratos\bin\Custom\applications\SwimmingDEMApplication\tests\tests_python_scripts\candelier_scripts\candelier_analysis.py", line 10, in <module>
    import candelier
  File "d:\a\Kratos\Kratos\bin\Custom\applications\SwimmingDEMApplication\tests\tests_python_scripts\candelier_scripts\candelier.py", line 2, in <module>
    import scipy.special
ModuleNotFoundError: No module named 'scipy'
d:\a\Kratos\Kratos\bin\Custom\applications//SwimmingDEMApplication/tests/test_SwimmingDEMApplication.py

@philbucher
Copy link
Member

cool now it works!

In order to add it to the CI the official maintainers should be know => #6412 (comment)
I guess this would be you and @maceligueta?

@GuillermoCasas
Copy link
Member Author

GuillermoCasas commented Jul 17, 2020

Yes, you assume correctly.

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

The @KratosMultiphysics/technical-committee approves adding the SwimmingDEM app to the CI

Nice work 👍

@GuillermoCasas GuillermoCasas merged commit 8d1764f into master Jul 17, 2020
@GuillermoCasas GuillermoCasas deleted the sdem/improving-tests branch July 17, 2020 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Continuous Integration related to Travis, Appveyor, ... Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants