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

Fix tests and address #36 #37

Merged
merged 13 commits into from
Feb 13, 2021
Merged

Fix tests and address #36 #37

merged 13 commits into from
Feb 13, 2021

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Feb 13, 2021

Purpose

This PR does two things:

  • Update the tests
    • moved them to outside the project directory so they are not installed
    • improved directory handling for the temporary directories used in testing
  • Closes Better handling of pdflatex compilation failure #36 by invoking subprocess.run instead of os.system

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Existing tests pass.

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@ewu63 ewu63 requested a review from a team as a code owner February 13, 2021 01:14
@ewu63 ewu63 requested a review from marcomangano February 13, 2021 01:14
@ewu63 ewu63 marked this pull request as draft February 13, 2021 01:14
@ewu63 ewu63 changed the title Fix tests and migrate to GHA Fix tests and address #36 Feb 13, 2021
@ewu63 ewu63 marked this pull request as ready for review February 13, 2021 02:32
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Why did you give up with GA?

Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Anyway, the tests work fine and the change are definitely an improvement, thanks for taking care of this!

@ewu63
Copy link
Collaborator Author

ewu63 commented Feb 13, 2021

After some discussions with @bernardopacini we decided it was best to do this on Azure to reduce code duplication.

@ewu63 ewu63 merged commit 9c59eb6 into master Feb 13, 2021
@ewu63 ewu63 deleted the fix-tests branch February 13, 2021 15:29
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.

Better handling of pdflatex compilation failure
2 participants