Skip to content

Conversation

@mmsqe
Copy link
Contributor

@mmsqe mmsqe commented Nov 19, 2025

Description

to avoid fail test like #830 #839


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

@mmsqe mmsqe requested a review from cloudgray November 19, 2025 13:48
@mmsqe
Copy link
Contributor Author

mmsqe commented Nov 19, 2025

@aljo242 actually need merge #840 1st

@cloudgray cloudgray mentioned this pull request Nov 20, 2025
3 tasks
@cloudgray
Copy link
Contributor

@mmsqe
This is quite a shocking discovery.

I can’t believe that common.U2560 had a non-zero value assigned to it and was causing interference between different test cases…

Thanks to this, we can remove the temporary workarounds in the other PRs (#837, #840) and finally address the root cause!

@mmsqe
Copy link
Contributor Author

mmsqe commented Nov 20, 2025

@mmsqe This is quite a shocking discovery.

I can’t believe that common.U2560 had a non-zero value assigned to it and was causing interference between different test cases…

Thanks to this, we can remove the temporary workarounds in the other PRs (#837, #840) and finally address the root cause!

Not sure why temporary, we need both fixes to ensure test isolation and avoid global mutation right?

@cloudgray
Copy link
Contributor

Not sure why temporary, we need both fixes to ensure test isolation and avoid global mutation right?

@mmsqe

  • Even taking test isolation into consideration, this situation seems highly abnormal and exceptional.
  • Under normal circumstances, when the app is initialized, the module account balance should be zero, and the very idea of handling a case where it is not zero feels incorrect.
  • Moreover, the balance-adjusting workaround is not applied to all accounts — it only applies to the precisebank module account. Since this workaround was introduced solely to deal with the underlying issue of the global variable being overwritten, I believe it is best to roll it back once that root cause is resolved.
  • I also think this is preferable because future developers who encounter this balance-adjusting helper function without understanding its context will have a hard time grasping why it exists in the first place.

@mmsqe mmsqe marked this pull request as ready for review November 20, 2025 15:19
@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.15%. Comparing base (5ca1990) to head (c4b72bb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
+ Coverage   65.03%   65.15%   +0.11%     
==========================================
  Files         317      317              
  Lines       21615    21608       -7     
==========================================
+ Hits        14057    14078      +21     
+ Misses       6365     6364       -1     
+ Partials     1193     1166      -27     

see 2 files with indirect coverage changes

🚀 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.

@aljo242
Copy link
Contributor

aljo242 commented Nov 20, 2025

@mmsqe @cloudgray good to merge?

Copy link
Contributor

@cloudgray cloudgray left a comment

Choose a reason for hiding this comment

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

LGTM!

FYI, I'll close PR #839 .
Because this pr solved problem clearly so we don't need PR #839 anymore.

good to merge?
@aljo242 Yes!

@mmsqe mmsqe added this pull request to the merge queue Nov 21, 2025
Merged via the queue into cosmos:main with commit cb68a1b Nov 21, 2025
22 checks passed
@mmsqe mmsqe deleted the fix_U2560 branch November 21, 2025 01:05
Sign up for free to 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.

3 participants