Skip to content

Remove subprocess calls from tests - #251

Merged
Sparks29032 merged 1 commit into
diffpy:mainfrom
Sparks29032:no-subprocess
Sep 2, 2025
Merged

Remove subprocess calls from tests#251
Sparks29032 merged 1 commit into
diffpy:mainfrom
Sparks29032:no-subprocess

Conversation

@Sparks29032

Copy link
Copy Markdown
Collaborator

New tests from #249 and #250 give errors on Windows. Running as shell command (in the case of Windows) is not recommended.

We don't need to call the subprocess since we can directly simulate a CLI call similar to the tests already present in test_morphapp.py.

@codecov

codecovBot commented Sep 2, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.92%. Comparing base (fc50526) to head (22f5ae3).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #251 +/- ##
=======================================
Coverage 99.92% 99.92% =======================================
Files 23 23 Lines 1354 1354 =======================================
Hits 1353 1353 Misses 1 1 
Files with missing linesCoverage Δ
tests/test_morphsqueeze.py100.00% <100.00%> (ø)
tests/test_refine.py100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Sparks29032

Copy link
Copy Markdown
CollaboratorAuthor

Merging since this is the exact same set of tests, but without explicit subprocess calls. Refer to test_morphapp.py tests.

@Sparks29032
Sparks29032 merged commit 9b44581 into diffpy:mainSep 2, 2025
5 checks passed
@sbillinge

Copy link
Copy Markdown
Contributor

yes, this is much better. It is also possible to import the main function from the entry-point app and run that the desired inputs. This is how we would normally do it to be platform independent.

Sign up for freeto 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.

2 participants

@Sparks29032@sbillinge