Skip to content
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

Jump to bottom

[master] Fix Flaky LazyLoaderRefreshFileMappingTest #64568

Merged
merged 2 commits into from
Aug 25, 2023
Merged

[master] Fix Flaky LazyLoaderRefreshFileMappingTest #64568

merged 2 commits into from
Aug 25, 2023

Conversation

cianyleow
Copy link
Contributor

@cianyleow cianyleow commented Jun 28, 2023 •
edited
Loading

What does this PR do?

Move creation of the lock to a private method to isolate mocking behaviour from main threading library.

What issues does this PR fix or reference?

Fixes: 64567

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

  • [N/A] Docs
  • [YES] Changelog - 64567.fixed.md
  • [YES] Tests written/updated

Commits signed with GPG?

Yes/No

@cianyleow cianyleow requested a review from a team as a code owner June 28, 2023 15:10
@cianyleow cianyleow requested review from cmcmarrow and removed request for a team June 28, 2023 15:10
@welcome
Copy link

welcome bot commented Jun 28, 2023

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 saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Fix Flaky LazyLoaderRefreshFileMappingTest [master] Fix Flaky LazyLoaderRefreshFileMappingTest Jun 28, 2023
Ch3LL
Ch3LL previously approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

This looks good but I certainly want to get @dwoz and @s0undt3ch 's review here

@Ch3LL Ch3LL requested review from dwoz and s0undt3ch June 28, 2023 18:43
s0undt3ch
s0undt3ch suggested changes Jun 28, 2023
View reviewed changes
tests/unit/test_loader.py Outdated Show resolved Hide resolved
@cianyleow cianyleow temporarily deployed to ci June 28, 2023 19:48 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci June 28, 2023 19:48 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci June 28, 2023 19:48 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci June 28, 2023 20:07 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci June 28, 2023 20:07 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci June 28, 2023 20:11 — with GitHub Actions Inactive
@cianyleow cianyleow requested a review from s0undt3ch June 29, 2023 16:02
@cianyleow cianyleow force-pushed the fix-flaky-lazy-loader-test branch from bd53834 to 1d29f20 Compare June 29, 2023 16:18
@cianyleow cianyleow force-pushed the fix-flaky-lazy-loader-test branch from 1d29f20 to 0739c05 Compare July 3, 2023 08:51
@cianyleow
Copy link
Contributor Author

@s0undt3ch thanks for the speedy reviews! I think I fixed the CI issues I noticed the other day, but will need a re-run to confirm

@cianyleow cianyleow temporarily deployed to ci July 3, 2023 16:54 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci July 3, 2023 16:54 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 00:41 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 00:44 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 00:57 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 01:07 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 02:40 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 02:42 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 05:45 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 05:45 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 05:45 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 05:45 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 05:46 — with GitHub Actions Inactive
@Ch3LL Ch3LL temporarily deployed to ci August 3, 2023 05:46 — with GitHub Actions Inactive
@cianyleow
Copy link
Contributor Author

Hey @Ch3LL not sure on the best way to get this merged as I've been in this cycle of either having failing tests or an out of date branch - would you like me to rebase on the latest master so that it can be re-run? The failing tests are related to Windows 2016 which look similar to other ones I've had flake on me in the past.

@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 18, 2023

Apologies, I have the 3007.0 label on this so it's on my radar. We are currently waiting on a merge forward for fixing all the tests. I'll make sure to follow up here to update the branch once that is merged into master.

@cianyleow cianyleow temporarily deployed to ci August 24, 2023 19:00 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 24, 2023 19:00 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 24, 2023 19:00 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 24, 2023 19:00 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 24, 2023 19:19 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 24, 2023 19:26 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 25, 2023 00:29 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 25, 2023 00:29 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 25, 2023 00:29 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 25, 2023 00:29 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 25, 2023 00:29 — with GitHub Actions Inactive
@cianyleow cianyleow temporarily deployed to ci August 25, 2023 00:29 — with GitHub Actions Inactive
@Ch3LL Ch3LL merged commit 5cb273c into saltstack:master Aug 25, 2023
@welcome
Copy link

welcome bot commented Aug 25, 2023

Congratulations on your first PR being merged! 🎉

@cianyleow cianyleow deleted the fix-flaky-lazy-loader-test branch August 29, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s0undt3ch s0undt3ch s0undt3ch approved these changes

@Ch3LL Ch3LL Ch3LL approved these changes

@cmcmarrow cmcmarrow Awaiting requested review from cmcmarrow cmcmarrow was automatically assigned from saltstack/team-core

@dwoz dwoz Awaiting requested review from dwoz

Assignees
No one assigned
Labels
Chlorine v3007.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TEST FAILURE] Flaky LazyLoaderRefreshFileMappingTest Test
3 participants
@cianyleow @Ch3LL @s0undt3ch