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.

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

Use different directories for cluster marker tests #1628

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

tillrohrmann
Copy link
Contributor

Multiple NamedTempFiles are created in the same directory. Therefore,
it happens that the cluster marker test can interfere with each other.
This commit separates the cluster marker tests by creating a temporary
directory in which the cluster marker file gets created.

This fixes #1626.

@AhmedSoliman AhmedSoliman self-requested a review June 14, 2024 08:14
Copy link
Contributor

@AhmedSoliman AhmedSoliman left a comment

Choose a reason for hiding this comment

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

Just checking, is it intentional to "leak" those temporary files after every test?

@tillrohrmann
Copy link
Contributor Author

Just checking, is it intentional to "leak" those temporary files after every test?

No, it is not. Now with tempdir() they should get removed after the test.

@tillrohrmann
Copy link
Contributor Author

Sorry for missing the point of into_path() not deleting the directory automatically. I've updated the PR accordingly. Now it should do things as intended, hopefully.

@tillrohrmann
Copy link
Contributor Author

The PR now seems to pass and should not leave temporary files around. Merging it.

Multiple NamedTempFiles are created in the same directory. Therefore,
it happens that the cluster marker test can interfere with each other.
This commit separates the cluster marker tests by creating a temporary
directory in which the cluster marker file gets created.

This fixes restatedev#1626.
@AhmedSoliman
Copy link
Contributor

We can try that. I'm still not sure I understand how does it fix the actual root cause though.

@tillrohrmann
Copy link
Contributor Author

We can try that. I'm still not sure I understand how does it fix the actual root cause though.

The problem was that we only created a temporary file for the cluster marker and not a temporary directory. The temporary files were all located in the same directory. When writing the new cluster marker, the process is that we first create the temporary cluster marker in the parent directory and then rename it. Since the temporary cluster marker has a fixed name, the different tests can interfere with each other since they share the same parent directory.

@tillrohrmann tillrohrmann merged commit 123ad9c into restatedev:main Jun 14, 2024
4 checks passed
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.

Flaky cluster_marker::tests::cluster_marker_is_updated when running it locally
2 participants