Skip to content

Conversation

@PGijsbers
Copy link
Contributor

@PGijsbers PGijsbers commented Nov 7, 2024

Change

Working on #378. No changes so far, other than refactoring and using Path.write_text instead of io.write.
Setting up PR to test flakiness in CI, as I can't reproduce failures locally.

How to Test

Checklist

  • Tests have been added or updated to reflect the changes, or their absence is explicitly explained.
  • Documentation has been added or updated to reflect the changes, or their absence is explicitly explained.
  • A self-review has been conducted checking:
    • No unintended changes have been committed.
    • The changes in isolation seem reasonable.
    • Anything that may be odd or unintuitive is provided with a GitHub comment explaining it (but consider if this should not be a code comment or in the documentation instead).
  • All CI checks pass before pinging a reviewer, or provide an explanation if they do not.

Related Issues

@PGijsbers
Copy link
Contributor Author

PGijsbers commented Nov 7, 2024

@andrejridzik Did this test fail consistently for you? Or at least consistently enough that you will have a good indication of whether or not things would be fixed. I cannot reproduce it at all and it now also not failing in CI... which leaves me at a loss. If you can reproduce it consistently, I would appreciate it if you could try and see if the test also fails for you on this branch.

edit: reproduced on CI now. Though not consistently which may make it hard to track down. Either way at least I know it's still present.

@PGijsbers PGijsbers marked this pull request as ready for review December 2, 2024 13:11
Copy link
Collaborator

@andrejridzik andrejridzik left a comment

Choose a reason for hiding this comment

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

I am still getting majority of failures with this setup. However, I was able to achieve 0% failure rate (n=100) when these two conditions are met:

  1. sleep duration set to 10**-1
  2. sleep moved to other location -> after file1.unlink()

Note: I got failure rate 11% when setting the sleep to 10**-2

os.makedirs(backup_dir)
create_test_files(original_dir, ["file1.txt", "file2.txt"])
backup()
file1.unlink()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some experiments and got most reliable results when adding the sleep after this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting... Perhaps we'll need both. I'll move the sleep to where you suggested it and see if that's good enough for CI too (because clearly behavior of CI/CD doesn't match your environment 1:1)

@PGijsbers
Copy link
Contributor Author

Needed both sleeps. I think should be good to go now @andrejridzik ?

@PGijsbers PGijsbers merged commit d0686b9 into develop Dec 11, 2024
1 check passed
@PGijsbers PGijsbers deleted the fix/flaky-test branch December 11, 2024 10:42
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