Fix Skipped Test Issue #904

XZTian64 · 2025-06-26T18:23:40Z

User description

Description

There were several logic problem during test. The test script ignored skipped cases for some special flags, and when combining % flag with other options will only count the skipped cases filtered by % flag.

Fixes #(issue) [optional]

Type of change

Please delete options that are not relevant.

Fixed the logic in toolchain/mfc/test/test.py script

Scope

This PR comprises a set of related changes with a common goal

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

Test A
Test B

Test Configuration:

What computers and compilers did you use to test this:

Checklist

I have added comments for the new code
I added Doxygen docstrings to the new code
I have made corresponding changes to the documentation (docs/)
I have added regression tests to the test suite so that people can verify in the future that the feature is behaving as expected
I have added example cases in examples/ that demonstrate my new feature performing as expected.
They run to completion and demonstrate "interesting physics"
I ran ./mfc.sh format before committing my code
New and existing tests pass locally with my changes, including with GPU capability enabled (both NVIDIA hardware with NVHPC compilers and AMD hardware with CRAY compilers) and disabled
This PR does not introduce any repeated code (it follows the DRY principle)
I cannot think of a way to condense this code and reduce any introduced additional line count

If your code changes any code source files (anything in `src/simulation`)

To make sure the code is performing as expected on GPU devices, I have:

Checked that the code compiles using NVHPC compilers
Checked that the code compiles using CRAY compilers
Ran the code on either V100, A100, or H100 GPUs and ensured the new feature performed as expected (the GPU results match the CPU results)
Ran the code on MI200+ GPUs and ensure the new features performed as expected (the GPU results match the CPU results)
Enclosed the new feature via nvtx ranges so that they can be identified in profiles
Ran a Nsight Systems profile using ./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR
Ran a Rocprof Systems profile using ./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace, and have attached the output file and plain text results to this PR.
Ran my code using various numbers of different GPUs (1, 2, and 8, for example) in parallel and made sure that the results scale similarly to what happens if you run without the new code/feature

PR Type

Bug fix

Description

Fixed skipped test case tracking logic in test filtering
Corrected handling of skipped cases when using percent flag
Added proper skipped case collection for range and filter operations

Changes walkthrough 📝

Relevant files

Bug fix

test.py `Fix skipped test case tracking logic` toolchain/mfc/test/test.py Added skipped case tracking for `--from`/`--to` range filtering Fixed skipped case collection in `--only` filter logic Added skipped case tracking for `--single` flag filtering Fixed skipped case accumulation when using `--percent` flag	+5/-1

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

qodo-merge-pro · 2025-06-26T18:24:03Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Logic Bug The skipped_cases calculation for --from/--to range filtering includes all cases not in the filtered range, but this may double-count cases that were already filtered out by previous filters. The logic should only add cases that were specifically excluded by the range filter. skipped_cases += [case for case in cases_ if case not in cases] bFoundTo = True Duplicate Logic The skipped_cases collection for no_examples filter duplicates the filtering logic instead of reusing the existing list comprehension. This violates DRY principle and could lead to inconsistencies if the filtering condition changes. skipped_cases += [case for case in cases if "Example" in case.trace] cases = [case for case in cases if not "Example" in case.trace]

qodo-merge-pro · 2025-06-26T18:24:40Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use UUID-based filtering for accuracy The skipped cases calculation is inefficient and potentially incorrect. It compares entire case objects which may not work as expected. Use set operations with UUIDs for accurate filtering. toolchain/mfc/test/test.py [38-42] if bFoundFrom and case.get_uuid() == ARG("to"): cases = cases[from_i:i+1] - skipped_cases += [case for case in cases_ if case not in cases] + selected_uuids = {case.get_uuid() for case in cases} + skipped_cases += [case for case in cases_ if case.get_uuid() not in selected_uuids] bFoundTo = True break Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that comparing `TestCase` objects directly with `not in` can be inefficient. Using a set of UUIDs for the check is more performant and robust, assuming UUIDs are unique. This is a good code quality and performance improvement.	Low
Update

sbryngelson · 2025-06-26T19:05:15Z

Please post an output in the PR showing that this works as expected for all the examples it effects, like --no-examples and ./mfc.sh test -f <..> and the --single case.

Thanks!

codecov · 2025-06-26T20:35:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.82%. Comparing base (1ab0f5e) to head (295be10).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #904   +/-   ##
=======================================
  Coverage   45.82%   45.82%           
=======================================
  Files          68       68           
  Lines       18656    18656           
  Branches     2247     2247           
=======================================
  Hits         8550     8550           
  Misses       8753     8753           
  Partials     1353     1353

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

XZTian64 · 2025-06-27T01:15:08Z

For the single flag, the fixed output is like

......
     4E0FBE72    939.80    3D -> IBM -> STL
     14975B87    410.12    3D -> Viscosity -> Bubbles -> QBMM
     D13FDB23    408.94    3D -> mixlayer_perturb
     AB6E5D10    483.39    3D -> mixlayer_perturb -> bubbles
     C5205982    589.88    3D -> Bubbles -> QBMM
     D33BD146    663.68    3D -> Bubbles -> QBMM -> Non-polytropic

  
 
Test Summary: 308 passed, 0 failed, 154 skipped.

 Skipped Cases

 1D -> weno_order=5 -> teno=T
 1D -> weno_order=7 -> teno=T
 1D -> 1 Fluid(s) -> riemann_solver=1 -> low_Mach=1
 1D -> 1 Fluid(s) -> riemann_solver=2 -> low_Mach=1
 1D -> 1 Fluid(s) -> riemann_solver=2 -> low_Mach=2
 ......

XZTian64 · 2025-06-27T01:52:40Z

Here is --no-examples output:

......
     4E0FBE72    1093.52    3D -> IBM -> STL
     D13FDB23    473.27    3D -> mixlayer_perturb
     E8372F50    409.36    3D -> Chemistry -> Perfect Reactor
     14975B87    536.99    3D -> Viscosity -> Bubbles -> QBMM
     AB6E5D10    557.19    3D -> mixlayer_perturb -> bubbles
     C5205982    871.33    3D -> Bubbles -> QBMM
     D33BD146    961.20    3D -> Bubbles -> QBMM -> Non-polytropic

  
 
Test Summary: 385 passed, 0 failed, 77 skipped.

 Skipped Cases

 1D -> Example -> brio_wu
 2D -> Example -> phasechange_bubble
 3D -> Example -> shockdroplet
 1D -> Example -> titarevtorro
 2D -> Example -> zero_circ_vortex
......

XZTian64 · 2025-06-27T02:24:33Z

mfc.sh --from ... --to ...

......
     DA54B209    135.58    3D -> Bubbles -> Polytropic -> bubble_model=2
     2060F55A    237.79    3D -> 2 Fluid(s) -> Viscous -> weno_Re_flux
     B7250A5B    238.62    3D -> 2 Fluid(s) -> Viscous -> weno_Re_flux -> weno_avg
     6F35CD77    179.97    3D -> Bubbles -> bubble_model=2
     07C33719    292.94    3D -> Cylindrical -> Viscous
     939D6718    369.35    3D -> Cylindrical -> Viscous -> weno_Re_flux
     09623DE3    369.37    3D -> Cylindrical -> Viscous -> weno_Re_flux -> weno_avg

  
 
Test Summary: 27 passed, 0 failed, 435 skipped.

 Skipped Cases

 2D -> IBM -> STL
 3D -> IBM -> STL
 1D -> bc=-1
 1D -> bc=-2
 1D -> bc=-4
 1D -> bc=-5
 1D -> bc=-6
 1D -> bc=-7
......

sbryngelson · 2025-06-27T03:05:57Z

what happens if you enable the MPI tests for single precision? and go to the MPI build obviously.

XZTian64 · 2025-06-27T17:09:29Z

what happens if you enable the MPI tests for single precision? and go to the MPI build obviously.

The following single-precision test was built with MPI, so it includes MPI cases, all of which passed. If MPI is not enabled, the skipped MPI cases will appear at the top of the Skipped Cases list. Here is the output.

......
Test Summary: 308 passed, 0 failed, 154 skipped.

 Skipped Cases

 1D -> weno_order=5 -> teno=T
 1D -> weno_order=7 -> teno=T
 1D -> 1 Fluid(s) -> riemann_solver=1 -> low_Mach=1
 1D -> 1 Fluid(s) -> riemann_solver=2 -> low_Mach=1
 1D -> 1 Fluid(s) -> riemann_solver=2 -> low_Mach=2
 ......

In comparison, the single-precision build without MPI skipped three MPI cases, as shown here

Test Summary: 305 passed, 0 failed, 157 skipped.

 Skipped Cases

 **1D -> 2 MPI Ranks
 2D -> 2 MPI Ranks
 3D -> 2 MPI Ranks**
 1D -> weno_order=5 -> teno=T
 1D -> weno_order=7 -> teno=T

Co-authored-by: Spencer Bryngelson <shb@gatech.edu> Co-authored-by: Spencer Bryngelson <sbryngelson@gmail.com>

Fix Skipped Test Issue

8528af5

XZTian64 requested a review from a team as a code owner June 26, 2025 18:23

qodo-merge-pro bot added the Review effort 2/5 label Jun 26, 2025

Modify for Reviewer

e1a6605

mpi single

9ce824e

sbryngelson self-requested a review as a code owner June 27, 2025 17:21

XZTian64 and others added 3 commits June 27, 2025 14:42

Merge branch 'master' into SkipTests

79f4692

Discard changes to .github/workflows/test.yml

889f159

Merge branch 'master' into SkipTests

295be10

sbryngelson approved these changes Jun 27, 2025

View reviewed changes

sbryngelson merged commit c1e5159 into MFlowCode:master Jun 28, 2025
40 of 47 checks passed

prathi-wind pushed a commit to prathi-wind/MFC-prathi that referenced this pull request Jul 13, 2025

Fix Skipped Test Issue (MFlowCode#904)

65e6545

Co-authored-by: Spencer Bryngelson <shb@gatech.edu> Co-authored-by: Spencer Bryngelson <sbryngelson@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Skipped Test Issue #904

Fix Skipped Test Issue #904

Uh oh!

XZTian64 commented Jun 26, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

qodo-merge-pro bot commented Jun 26, 2025

Uh oh!

qodo-merge-pro bot commented Jun 26, 2025 •

edited

Loading

Uh oh!

sbryngelson commented Jun 26, 2025

Uh oh!

codecov bot commented Jun 26, 2025 •

edited

Loading

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

sbryngelson commented Jun 27, 2025

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

Uh oh!

Uh oh!

Fix Skipped Test Issue #904

Fix Skipped Test Issue #904

Uh oh!

Conversation

XZTian64 commented Jun 26, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Type of change

Scope

How Has This Been Tested?

Checklist

If your code changes any code source files (anything in src/simulation)

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-merge-pro bot commented Jun 26, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

sbryngelson commented Jun 26, 2025

Uh oh!

codecov bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

sbryngelson commented Jun 27, 2025

Uh oh!

XZTian64 commented Jun 27, 2025

Uh oh!

Uh oh!

Uh oh!

XZTian64 commented Jun 26, 2025 •

edited by qodo-merge-pro bot

Loading

If your code changes any code source files (anything in `src/simulation`)

qodo-merge-pro bot commented Jun 26, 2025 •

edited

Loading

codecov bot commented Jun 26, 2025 •

edited

Loading