Skip to content

Conversation

@AhmadYasser1
Copy link

What does this PR do?

Performance optimization

What issues does this PR fix or reference?

Addresses TODO comments in tests/unit/test_loader.py about .pyc files being slower on reload (~3x)

Previous Behavior

Test loader was writing .pyc files and then removing them with remove_bytecode() calls, causing performance issues when modules were reloaded quickly during test execution

New Behavior

Test loader now prevents .pyc files from being written in the first place using sys.dont_write_bytecode = True, resulting in ~3x faster test execution and reduced I/O operations

Merge requirements satisfied?

Commits signed with GPG?

No

Additional Details

Technical Changes

  • Set sys.dont_write_bytecode = True in LazyLoaderReloadingTest.setUp()
  • Restore original setting in tearDown() to avoid side effects
  • Remove redundant remove_bytecode() calls in update_module() and rm_module()
  • Update comments to reflect the new optimization approach

Performance Impact

  • ~3x faster test execution (as mentioned in TODO comments)
  • Reduced I/O operations during test runs
  • Cleaner test execution without temporary .pyc files

Code Quality

  • Addresses technical debt (TODO comments in codebase)
  • Removes redundant operations
  • Improves maintainability with clearer comments

- Rename misleading variable 'typo_warning' to 'missing_vars_warning'
- Update warning message to be more accurate about missing variables
- Fix typo in warning message: 'dissolve' -> 'resolve'
- Improve code readability and maintainability

The variable name 'typo_warning' was misleading as it's actually about
missing variables in _syspaths.py, not typos. This change makes the
code more self-documenting and the warning message more helpful.
- Set sys.dont_write_bytecode = True in LazyLoaderReloadingTest.setUp()
- Restore original setting in tearDown() to avoid side effects
- Remove redundant remove_bytecode() calls in update_module() and rm_module()
- Addresses TODO comments about .pyc files being slower on reload (~3x)

This optimization prevents .pyc files from being written during test execution,
which was causing performance issues when modules were reloaded quickly.
The original TODO suggested this approach as being much faster than writing
and then removing .pyc files.
@AhmadYasser1 AhmadYasser1 requested a review from a team as a code owner October 21, 2025 00:27
@welcome
Copy link

welcome bot commented Oct 21, 2025

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We're glad you've joined our community and look forward to doing awesome things with you!

Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

Could you create a changelog for this, please?

@twangboy twangboy added the test:full Run the full test suite label Oct 31, 2025
@twangboy twangboy added this to the Argon v3008.0 milestone Oct 31, 2025
@twangboy
Copy link
Contributor

Does this affect the 3006.x branch as well? If so, please create against that branch. Then we can get speed improvements across all branches.

@AhmadYasser1
Copy link
Author

AhmadYasser1 commented Nov 1, 2025

I’ve opened a backport PR targeting 3006.x with the same changes.

New PR: #68444

This was created by cherry-picking the commits from this PR onto 3006.x and pushing to my fork (branch: pr-68412-3006x).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants