-
Notifications
You must be signed in to change notification settings - Fork 42
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
Updated CI #25
Updated CI #25
Conversation
@JustinSGray do you think it's worthwhile to keep the example builds as additional tests? |
os.system('python {}.py'.format(f)) | ||
self.assertTrue(os.path.isfile(f + '.tikz')) | ||
self.assertTrue(os.path.isfile(f + '.tex')) | ||
self.assertTrue(os.path.isfile(f + '.pdf')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should only be asserted, if pdflatex is installed, otherwise it will fail.
Like here: https://github.com/onodip/OpenMDAO-XDSM/blob/master/omxdsm/tests/test_xdsm_viewer.py#L167
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good point, will borrow your approach from the XDSM plugin, thanks for pointing this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm should the logic be changed to:
self.assertTrue(not pdflatex or (pdflatex and os.path.isfile(f + '.pdf')))
?
The way it's done in the referenced repo would mean that if pdflatex is installed, then it doesn't actually check isfile
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be equivalent. Either not pdflatex
is True, in this case the statement after the or
is not even evaluated (since the PDF cannot be possibly created, it is not an error, if the file does not exist, so this is okay). If not pdflatex
is False - so pdflatex
is installed, then it checks the file. In your line the or is only reached, if pdflatex
is True, so it does not have to be checked again, that pdflatex
is True.
I added below a snippet, which shows, that the two approaches give the same result in all combinations. s3
is there to clarify the precedence. a=pdflatex
, b=os.path.isfile(f + '.pdf')
for a in (True, False):
for b in (True, False):
print(f'{a=}')
print(f'{b=}')
s1 = not a or b
s2 = not a or (a and b)
s3 = (not a) or b
print(f'{s1=}')
print(f'{s2=}')
print(f'{s3=}')
print('\n')
a=True
b=True
s1=True
s2=True
s3=True
a=True
b=False
s1=False
s2=False
s3=False
a=False
b=True
s1=True
s2=True
s3=True
a=False
b=False
s1=True
s2=True
s3=True
Note: I found another minor logical error in the code. If pdflatex is not installed, and the file is there, the assertion is okay. This could be suspicious (maybe the file was there before the test, and therefore the test would always pass), but since a temporarily directory is used for the tests, that cannot be the case. The totally correct way would be (a and b) or ((not a) and (not b))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right that makes sense, thanks for that. Cheers!
Maybe it's still useful to keep building the examples? I can add that back if we want.