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

CHORE: Use matplotlib Agg backend when testing #4702

Closed
wants to merge 7 commits into from

Conversation

SMoraisAnsys
Copy link
Collaborator

Multiple recent PRs struggled because figures get shown in the CICD and one would have to close them manually (which does not work through CICD). This leads to tests failing due to time out as no one closes the figures manually.

Here we use Agg backend which is a non-GUI backend and will avoid future PR to struggle with this issue.

For example, this happened in #4626
Also related to #4672

Note: For some reasons it seems that changes have been made
which lead to figures showing when running CICD. This leads
to tests time out as no one closes the figures manually.
Here we use Agg backend which is a non-GUI backend.
@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added documentation maintenance Package and maintenance related labels May 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.88%. Comparing base (a67460f) to head (04f6ebf).

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4702       +/-   ##
===========================================
- Coverage   80.41%   33.88%   -46.53%     
===========================================
  Files         121      121               
  Lines       55100    55100               
===========================================
- Hits        44308    18671    -25637     
- Misses      10792    36429    +25637     

@SMoraisAnsys SMoraisAnsys marked this pull request as draft May 21, 2024 12:01
@SMoraisAnsys
Copy link
Collaborator Author

Since this changes might also impact the documentation, we might want to rename the environment variable a bit and see it's impact with the documentation.

gmalinve
gmalinve previously approved these changes May 22, 2024
@SMoraisAnsys SMoraisAnsys dismissed stale reviews from gmalinve and Samuelopez-ansys via 40e6e10 May 22, 2024 08:24
@SMoraisAnsys SMoraisAnsys changed the title CHORE: Use matplotlib Agg backend in CICD CHORE: Use matplotlib Agg backend when testing May 22, 2024
@SMoraisAnsys
Copy link
Collaborator Author

Changes: I removed the use of the environment variable 'ON_CI' and added a fixture instead. This will ensure that the documentation is not impacted.

@Samuelopez-ansys If you want, we can update the test to remove the show=False argument. Now, even if show=True, the tests should be passing correctly. Let me know if you want to extend this PR with those changes.

@SMoraisAnsys SMoraisAnsys marked this pull request as ready for review May 22, 2024 08:27
@Samuelopez-ansys
Copy link
Member

@SMoraisAnsys Yes please, remove the show=False, good job!

MaxJPRey
MaxJPRey previously approved these changes May 22, 2024
Copy link
Collaborator

@MaxJPRey MaxJPRey left a comment

Choose a reason for hiding this comment

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

LGTM.

@SMoraisAnsys SMoraisAnsys dismissed stale reviews from MaxJPRey and Samuelopez-ansys via fb0cfb7 May 23, 2024 08:51
@SMoraisAnsys
Copy link
Collaborator Author

Removing show=False proved to have impact on the tests and led me to notice "misses" in our recent refactoring.
This PR should stay as a draft as long as #4723 is not merged.
Then tests should be updated accordingly.

@SMoraisAnsys
Copy link
Collaborator Author

I'll close this PR as adding those changes increases our tests by a huge overhead.
However, it was not entirely a waste of time as it allowed to catch #4730 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Package and maintenance related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants