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

FEAT: Add AEDT File Management tests #5879

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

Samuelopez-ansys
Copy link
Member

Description

Please provide a brief description of the changes made in this pull request.

Issue linked

Close #5756

Checklist

  • I have tested my changes locally.
  • I have added necessary documentation or updated existing documentation.
  • I have followed the coding style guidelines of this project.
  • I have added appropriate tests (unit, integration, system).
  • I have reviewed my changes before submitting this pull request.
  • I have linked the issue or issues that are solved by the PR if any.
  • I have agreed with the Contributor License Agreement (CLA).

marwane rachad and others added 2 commits March 7, 2025 08:10
@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 the enhancement New features or code improvements label Mar 7, 2025
@Samuelopez-ansys Samuelopez-ansys enabled auto-merge (squash) March 7, 2025 07:26
Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 72.00000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 84.99%. Comparing base (4d786bf) to head (c5dfd1b).
Report is 2 commits behind head on main.

❌ Your patch check has failed because the patch coverage (72.00%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5879      +/-   ##
==========================================
+ Coverage   84.92%   84.99%   +0.07%     
==========================================
  Files         161      161              
  Lines       62156    62179      +23     
==========================================
+ Hits        52785    52851      +66     
+ Misses       9371     9328      -43     
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SMoraisAnsys SMoraisAnsys disabled auto-merge March 7, 2025 08:28
Copy link
Collaborator

@SMoraisAnsys SMoraisAnsys left a comment

Choose a reason for hiding this comment

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

Overall LGTM but I left a few minor comments. Thanks for making the migration @Samuelopez-ansys and thanks for the propositio @Marwane-20

@@ -47,11 +49,17 @@ def read_info_fromcsv(projdir, name):
list
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we deprecate this method and create a new one (with the same content) named read_info_form_csv ?

Comment on lines +58 to +60
content = content_bytes.decode("utf-8")
except UnicodeDecodeError:
content = content_bytes.decode("latin1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would revert to the previous statement unless we are sure that our users have data that can be decoded with "utf-8" or "latin1. @Samuelopez-ansys Do you know to what extent are we supposed to be compatible with other encoding ? Are we limited to "utf-8" ? If that's the case, we can only try that encoding.


filename = projdir + "//" + name
# Construct the filename using pathlib.
filename = str(Path(projdir) / name)
listmcad = []
with open_file(filename, "rb") as csvfile:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't open_file compatible with Path now ?

Comment on lines +151 to +152
view_str = u"Drawings[" + str(len(solid_list)) + u": " + str(solid_list).strip("[")
s = pattern.sub(r"\1" + view_str + r"\3", content)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
view_str = u"Drawings[" + str(len(solid_list)) + u": " + str(solid_list).strip("[")
s = pattern.sub(r"\1" + view_str + r"\3", content)
view_str = f"Drawings[{len(solid_list)}: {str(solid_list).strip('[')}"
s = pattern.sub(fr"\1{view_str}\3", content)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think u"..." are needed

Comment on lines +164 to +165
logging.error("change_objects_visibility: Error encountered - %s", e)
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logging.error("change_objects_visibility: Error encountered - %s", e)
return False
raise AEDTRuntimeError("Failed to restrict visibility to specified solids.") from e

I think that this will required an additional import

Comment on lines +167 to +168
logging.error("change_objects_visibility: Project %s is still locked.", origfile)
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logging.error("change_objects_visibility: Project %s is still locked.", origfile)
return False
raise AEDTRuntimeError(f"Project {origfile} is still locked.")

Comment on lines +237 to +238
logging.error("change_model_orientation: Error encountered - %s", e)
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logging.error("change_model_orientation: Error encountered - %s", e)
return False
raise AEDTRuntimeError("Failed to change model orientation.") from e

Comment on lines +240 to +241
logging.error("change_model_orientation: Project %s is still locked.", origfile)
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logging.error("change_model_orientation: Project %s is still locked.", origfile)
return False
raise AEDTRuntimeError(f"Project {origfile} is still locked.")


# Call the function.
result = clean_proj_folder(proj_dir.as_posix(), "project")
assert result is True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert result is True
assert result

@@ -0,0 +1,123 @@
# -*- coding: utf-8 -*-
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this file test should be moved to tests/unit since we are not testing the method with a subset of other modules / classes

solid_list = ["solid1", "solid2"]
result = change_objects_visibility(str(origfile), solid_list)
# Since the regex is applied on bytes, an error occurs and the function returns False.
assert result is False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert result is False
assert not result


result = change_model_orientation(str(origfile), "+X")
# The function returns False on error.
assert result is False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert result is False
assert not result

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve coverage aedt_file_management file
3 participants