-
Notifications
You must be signed in to change notification settings - Fork 1
EntList Integration Tests #34
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
Conversation
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.
Pull request overview
This PR adds integration tests for the EntList class to verify its functionality with real Moldflow Synergy COM objects. The tests cover entity selection, size checking, string conversion, and entity retrieval operations.
Key changes:
- Created comprehensive integration tests for
EntListclass methods includingselect_from_string,select_from_predicate,convert_to_string, andentity - Added test data constants for three different model types (dd_model, midplane_model, 3d_model)
- Registered
ent_listpytest marker for test organization
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/api/unit_tests/test_unit_ent_list.py | Added ent_list pytest marker to existing unit test class |
| tests/api/integration_tests/test_integration_ent_list.py | New integration test file with comprehensive test coverage for EntList functionality |
| tests/api/integration_tests/data/set_fields.py | Added test data constants for entity list integration tests |
| pytest.ini | Registered ent_list pytest marker |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Is it possible to also test |
Done, missed the function with original testing |
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.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| TEST_ENTITY_LIST_STRING = { | ||
| "dd_model": { | ||
| "items": "T56 T57", |
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.
There should be a reference to the underlying model.
miraladsk
left a comment
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 have few comments reg Integration architecture
- For any newly created test, I would love to see only addition (no modification)
- I don't see a point of maintaining zip file, any addition to zip impacts all existing test cases
- Test specific data should be managed in specific folder (test name as folder)
- Every validation of tests should be within test specific folder
@osinjoku Let me know if you have some opinion.
cc: @sankalps0549
I will be addressing the points and raising another PR for the required infra changes. Drafted this PR for the time being. About storing the study files, can we get a final decision @osinjoku @miraladsk, do we want to keep them zipped or unzipped. |
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.
Pull request overview
Copilot reviewed 8 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/api/integration_tests/test_suite_ent_list/generate_expected_data_ent_list.py
Outdated
Show resolved
Hide resolved
tests/api/integration_tests/test_suite_ent_list/test_integration_ent_list.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
| "time": "06:58:32 UTC", | ||
| "build_number": "49.1.198", | ||
| "version": "2026" | ||
| }, |
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 would consider this as an exception case, Going ahead, we should not see any changes in this file, Please improve infra
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.
Will improve in the future
| BEGIN PROPERTIES | ||
| END PROPERTIES | ||
| Last Write Time: Thu Nov 6 18:28:23 2025 | ||
| Last Write Time: Tue Dec 2 21:18:16 2025 |
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.
Why the changes in sdy & mpi?
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.
As noted in the PR description, this change updates all study files in the meshed_studies project (3d_model, dd_model, and midplane_model) by adding a new saved list to the study.
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.
Is it just for this PR or going to happen for every subsequent PR?
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.
If the study doesn't have something that we require for the tests, it might, but we don't expect to see this too often.
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.
In that case, I would always expect a new study, Either you create study with full features or just keep on adding new,
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.
My concern is that creating new study files for each requirement will add a lot of extra files to the repo unnecessarily. We should only add new ones when clearly needed.
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.
True, why we can't anticipate all the required things in sdy (we already know the API list) and create a full-fledged study upfront?
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.
That would require a separate, deeper review of every API, and in many cases the requirements aren’t immediately obvious. Instead of dedicating extra time solely for that, we’re updating the study files alongside the test development. Since all of this work is happening on a feature branch, by the time it’s merged into main we’ll end up with the finalized versions of the sdy and mpi files.
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.
At least initial spike will reduce the number of changes in sdy file. My only worry is impact. When there is a change in sdy, all existing test cases are impacted and may fail and reviewer is not going to replay them on their machine
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.
We don’t anticipate frequent updates to the study files, so it may be worth having this discussion again only if it starts occurring more often.
| from unittest.mock import Mock | ||
| import pytest | ||
| from moldflow import EntList, Predicate | ||
|
|
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.
Why this file is shown as modified and not newly created?
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.
We previously had unit tests without class-specific labels. Now that we’re introducing labels, we’re going back and adding them to the existing tests as well.
Description
This PR introduces the integration tests for the
EntListclass.Changes in this PR:
meshed_studiesproject, i.e.3d_model,dd_model,midplane_model. The change includes adding a new saved list to the study.test_suite_ent_listtest_integration_ent_list.pyent_listent_listpytest marker to thetest_unit_ent_list.pyclassType of change
Please delete options that are not relevant.
Checklist
Please delete options that are not relevant.
Data Generation
Testing
Added Integration Tests for the EntList class
Test Run:
Additional Notes
N/A