Skip to content

VZ print error message #79

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.

Sign up for GitHub

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

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

VZ print error message #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e851cd2
add messaging for run failure
vickimzhang Mar 3, 2026
6230104
edit testing
vickimzhang Mar 3, 2026
8511937
remove unnecessary f-string in testing
vickimzhang Mar 3, 2026
7d78d05
add error message for run failure and test
vickimzhang Apr 1, 2026
1a1f0e9
merge changes from main
vickimzhang Apr 1, 2026
d2fcd3c
add testing for run failure
vickimzhang Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 28 additions & 13 deletions pysyncrosim/scenario.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,41 +602,56 @@ def run(self, copy_external_inputs=False):
if copy_external_inputs is True:
args += ["--copyextfiles=yes"]

try:
error_msg = None

try:
print(f"Running Scenario [{self.sid}] {self.name}")
result = self.library.session._Session__call_console(args)

if result.returncode == 0:
print("Run successful")
self.library.session._Session__call_console(args)

except RuntimeError as e:
# TODO: add handling when the error message contains "You must be signed in" or "There has been an issue with your SyncroSim license file"


print(e)
if "You must be signed in" in str(e):
error_msg = ("you must be signed in to SyncroSim. "
"Use session.sign_in() to sign in.")

elif "There has been an issue with your SyncroSim license file" in str(e):
error_msg = "there has been an issue with your SyncroSim license file."

else:
error_msg = str(e)

finally:

# Reset Project Scenarios
self.project._Project__scenarios = None

# Reset results
self.__results = None

# Retrieve Results Scenario ID
# Also resets scenarios and results info
results_df = self.results()

if (not results_df.empty):

result_id = results_df["ScenarioId"].values[-1]


if error_msg is not None:
raise RuntimeError(f"Run failed for Scenario [{result_id}] "
f"{self.name}: {error_msg}")
else:
print("Run successful")

# Return Results Scenario
result_scn = self.library.scenarios(project=self.project,
name=None,
sid=result_id)

return result_scn

elif error_msg is not None:
raise RuntimeError(f"Run failed for Scenario [{self.sid}] "
f"{self.name}: {error_msg}")
Comment on lines +652 to +654
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Function may return None implicitly when results are empty and no error occurred.

When results_df.empty is True and error_msg is None, the function exits without an explicit return, implicitly returning None. This doesn't match the docstring which states it returns a Scenario. Consider whether this edge case should print a warning or return something more explicit.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pysyncrosim/scenario.py` around lines 652 - 654, The function currently falls
through (implicit None) when results_df.empty is True and error_msg is None;
update the end-of-run handling so it returns an explicit Scenario object and
warns instead of returning None: when results_df.empty and error_msg is None,
emit a warning (e.g., using warnings.warn with context including self.sid and
self.name) and then return self (the Scenario) so the return type matches the
docstring; adjust the block surrounding results_df.empty and error_msg checks
(referencing results_df.empty, error_msg, self.sid, self.name, and the Scenario
return) to implement this explicit warning+return.

Copy link
Copy Markdown
Collaborator Author

@vickimzhang vickimzhang Apr 1, 2026

Choose a reason for hiding this comment

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

this edge case where no results scenario is created after a run failure should never happen

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 1, 2026

Choose a reason for hiding this comment

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

@vickimzhang, thanks for the clarification! If SyncroSim always produces a result scenario on a successful run, then that implicit None path is indeed unreachable and the current logic is correct. I'll note this for future reviews.

✏️ Learnings added 
Learnt from: vickimzhang
Repo: syncrosim/pysyncrosim PR: 79
File: pysyncrosim/scenario.py:652-654
Timestamp: 2026-04-01T14:26:18.541Z
Learning: In `pysyncrosim/scenario.py`, the `Scenario.run()` method's `finally` block handles two branches after checking `results_df`: (1) results exist, and (2) no results with an error. The case where `results_df.empty` is `True` and `error_msg` is `None` (implicit `None` return) is intentionally unreachable — SyncroSim always creates a result scenario on a successful run, so a successful run will never produce an empty results DataFrame.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


def run_log(self):
"""
Expand Down
23 changes: 19 additions & 4 deletions tests/test_pysyncrosim.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@
session_path = None
test_lib_path = os.path.join(temp_path.name, "stsimLibrary.ssim")
lib_name = "spatial-example.ssim"
git_repo_path = "C:/Users/VickiZhang/Documents/GH_ApexRMS"
# git_repo_path = "C:/gitprojects"
git_repo_path = "C:/GH_ApexRMS"
lib_path = os.path.join(git_repo_path, "pysyncrosim/tests", lib_name)
lib_backup_path = os.path.join(git_repo_path, "pysyncrosim/tests", "spatial-example.ssimbak")
Comment on lines +15 to 17
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 1, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Hardcoded absolute path reduces test portability.

The path C:/GH_ApexRMS is Windows-specific and tied to a particular machine's directory structure. This will cause test failures on other developers' machines or CI environments. Consider using environment variables or a configuration file for the repository path.

🛠️ Suggested fix using environment variable 
-git_repo_path = "C:/GH_ApexRMS"
+git_repo_path = os.environ.get("PYSYNCROSIM_TEST_REPO_PATH", "C:/GH_ApexRMS")

Alternatively, use a path relative to the test file:

-git_repo_path = "C:/GH_ApexRMS"
+git_repo_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
git_repo_path = "C:/GH_ApexRMS"
lib_path = os.path.join(git_repo_path, "pysyncrosim/tests", lib_name)
lib_backup_path = os.path.join(git_repo_path, "pysyncrosim/tests", "spatial-example.ssimbak")
git_repo_path = os.environ.get("PYSYNCROSIM_TEST_REPO_PATH", "C:/GH_ApexRMS")
lib_path = os.path.join(git_repo_path, "pysyncrosim/tests", lib_name)
lib_backup_path = os.path.join(git_repo_path, "pysyncrosim/tests", "spatial-example.ssimbak")
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/test_pysyncrosim.py` around lines 15 - 17, The test uses a hardcoded
Windows-only path in git_repo_path which breaks portability; replace usage in
git_repo_path, lib_path, and lib_backup_path so the repository root is
discovered dynamically (for example read from an environment variable like
REPO_ROOT via os.environ.get('REPO_ROOT') with a sensible fallback) or compute
it relative to the test file using os.path.dirname(__file__) and os.path.join to
construct lib_path and lib_backup_path; update the references in the test to use
this dynamic repo root so the tests run on different machines and CI.


Expand Down Expand Up @@ -932,6 +931,22 @@ def test_scenario_run_and_results():
assert not isinstance(myScenario.run_log(), pd.DataFrame)

myLibrary.delete(force=True)

def test_scenario_run_failure():

mySession = ps.Session(session_path)
mySession.restore(lib_backup_path)

myLibrary = ps.library(name=lib_path,
session=mySession,
force_update=True)

myScenario = myLibrary.scenarios(name="My Scenario")
myFailScenario = myScenario.copy(name="My Scenario - Failure Test")
myFailScenario.delete(force=True)

with pytest.raises(RuntimeError, match="Run failed for Scenario"):
myFailScenario.run()

def test_scenario_copy_dep_delete():

Expand Down Expand Up @@ -1026,8 +1041,8 @@ def test_scenario_copy_dep_delete():

# Test delete
myNewScn.delete(force=True)
emptyResScn = myNewScn.run()
assert emptyResScn is None
with pytest.raises(RuntimeError, match = "Run failed for Scenario"):
myNewScn.run()

# Delete other scenarios
sameNameScn.delete(force=True)
Expand Down