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

Fixing return of incorrect subprocess #3562

Merged
merged 6 commits into from
Jan 30, 2019
Merged

Conversation

roigcarlo
Copy link
Member

This fixes the run tests script not returning any error code while failing to execute a subprocess due to the command being invalid. Additionally I added to return an error if the subprocess can be initiated but the command fails due to wrong number of arguments (which should never happen)

I also fixed a warning with the imp lib being deprecated. This should increase compatibility with python 2.x and 3.3 or lower.

Finally, now all executions timed or not, go through the same pipeline to prevent future errors like this.

@philbucher
Copy link
Member

Nice work!
Are you working on more updates of the testing? I am asking bcs I am also currently trying to better integrate the c-tests (remember the issue we have).
Just asking to prevent merging struggles

@roigcarlo
Copy link
Member Author

roigcarlo commented Dec 4, 2018

Hi @philbucher, we plan to integrate support for mpi, but honestly I don't see it done before christmas

@philbucher
Copy link
Member

ok thx

@@ -42,7 +40,7 @@ def GetModulePath(module):

'''

return imp.find_module(module)[1]
return os.path.dirname(KtsMp.__file__)
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 no longer general I'd say => GetModulePath should now be named GetKratosMultiphysicsPath

@pooyan-dadvand
Copy link
Member

Just to say that it works for me and now reports correctly an error in subprocess

@philbucher
Copy link
Member

will this be merged this year?
Asking bcs I have some developments touching the same files

@philbucher
Copy link
Member

@roigcarlo I used the new functions from #3738

Also I noticed that if there is an error in one of the python-tests, running of all tests is aborted! (unrelated to what I changed)
E.g. if there is an error in the core it doesn't run the tests in the apps
or if there is an error in an app it won't run the tests in the remaining apps

@roigcarlo
Copy link
Member Author

roigcarlo commented Jan 4, 2019

@philbucher Agree with the changes.

Also, the premature exit is caused in one of the changes I introduced in this PR, but honestly I don't remember if it was our intention or a bug, because its even stated that the program should exit. But I find it odd too

This is the change:

if self.exitCode != 0:
    # If thread terminated with an error, propagate and terminate
    sys.exit(self.exitCode)

Let me speak with @pooyan-dadvand before reverting it

@roigcarlo
Copy link
Member Author

Hi @philbucher, I spoke with @pooyan-dadvand and he agreed to let the other tests continue, I will change it this afternoon

@philbucher
Copy link
Member

ping

@philbucher
Copy link
Member

ping :)

@pooyan-dadvand
Copy link
Member

I assume that the ping is for @roigcarlo

@philbucher
Copy link
Member

I assume that the ping is for @roigcarlo

indeed it is :)

@roigcarlo
Copy link
Member Author

Give me one sec

@roigcarlo
Copy link
Member Author

Done

@philbucher
Copy link
Member

Thanks @roigcarlo I am testing it now

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.

works again, thx!

@roigcarlo roigcarlo merged commit ce36752 into master Jan 30, 2019
@roigcarlo roigcarlo deleted the ci/fixing-error-code branch January 30, 2019 20:01
@philbucher philbucher mentioned this pull request Mar 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.

3 participants