Skip to content

Test specific parts of SymPy #196

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

Merged
merged 3 commits into from
Sep 30, 2017
Merged

Test specific parts of SymPy #196

merged 3 commits into from
Sep 30, 2017

Conversation

ShikharJ
Copy link
Member

No description provided.

@ShikharJ ShikharJ requested a review from isuruf September 29, 2017 14:52
@@ -1,3 +1,10 @@
import symengine
if not symengine.test():
raise Exception('Tests failed')

print('Testing SYMPY')
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to test this only on one or two jobs. (A python 2 and 3).

@@ -1,3 +1,23 @@
import os
TEST_SYMPY = os.getenv('TEST_SYMPY', '0')
Copy link
Member

Choose a reason for hiding this comment

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

Change this to TEST_SYMPY = os.getenv('TEST_SYMPY', False), otherwise the condition will always be true.

import symengine
if not symengine.test():
raise Exception('Tests failed')

try:
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this try statement and move the lines below to under if TEST_SYMPY?

if [[ "${TEST_SYMPY}" == "yes" ]]; then
export TEST_SYMPY = 1;
fi

Copy link
Member

Choose a reason for hiding this comment

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

4 lines above are unnecessary

@ShikharJ
Copy link
Member Author

Ping @isuruf.

@isuruf
Copy link
Member

isuruf commented Sep 30, 2017

@certik, this adds less than a minute of testing time to 2 jobs.

@isuruf isuruf merged commit c629ca0 into symengine:master Sep 30, 2017
@isuruf
Copy link
Member

isuruf commented Sep 30, 2017

Thanks @ShikharJ for the PR

@ShikharJ ShikharJ deleted the Test branch September 30, 2017 16:20
@certik
Copy link
Contributor

certik commented Sep 30, 2017

Thanks, this looks great. There are some warnings from Teuchos:

https://travis-ci.org/symengine/symengine.py/jobs/281611351#L1697

this might mean some kind of a bug, we should look into it.

@isuruf
Copy link
Member

isuruf commented Sep 30, 2017

@certik, those warnings are unrelated. They've been there for ages. The 16 warnings are because of the 16 variables we create in test_var.py where we use var to inject symbols to the parent frame.

@certik
Copy link
Contributor

certik commented Sep 30, 2017

Ok, I created an issue for the warnings: #197.

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