Skip to content

Fix Windows unit test failing because of DataStore threading issue #1542

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

Merged
merged 6 commits into from
Oct 14, 2024

Conversation

SimonMarquis
Copy link
Contributor

This PR fixes issues on Windows environments, where DataStore threading is known to be buggy.
A fix has been awaited for months now, but still remains...
Replacing the default DataStore implementation with an in-memory fake avoids having to deal with file synchronization that lead to the issue.

Here is an example of such errors that appears on Windows env:

> Task :core:datastore:testDemoDebugUnitTest

com.google.samples.apps.nowinandroid.core.datastore.NiaPreferencesDataSourceTest > userShouldHideOnboarding_unfollowsLastTopic_shouldHideOnboardingIsFalse FAILED
    java.io.IOException: Unable to rename C:\Users\RUNNER~1\AppData\Local\Temp\junit11942129073694190937\user_preferences_test.pb.tmp.This likely means that there are multiple instances of DataStore for this file. Ensure that you are only creating a single instance of datastore for this file.

It fixes both of these issues that can be closed:

Here is a before/after (red/green) on a Windows env in GHA:

@SimonMarquis
Copy link
Contributor Author

@dturner I don't know exactly how many Windows users there are in the wild building this project but I'm pretty sure it's not zero (I'm one of them). It'd great to fix unit tests for them :)

@lihenggui
Copy link
Contributor

@dturner I don't know exactly how many Windows users there are in the wild building this project but I'm pretty sure it's not zero (I'm one of them). It'd great to fix unit tests for them :)

It is definitely not zero. I am one of the developers using Windows to build this project. : )

@SimonMarquis SimonMarquis force-pushed the in-memory-datastore branch from 41a4bb4 to 45b0c2a Compare July 11, 2024 21:59
Copy link
Contributor

@Jaehwa-Noh Jaehwa-Noh left a comment

Choose a reason for hiding this comment

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

image

Congratulation! Works on Window!

@SimonMarquis
Copy link
Contributor Author

@dturner PTAL 🙏

@SimonMarquis
Copy link
Contributor Author

SimonMarquis commented Sep 30, 2024

I'll fix the badging issue in another PR.

Copy link
Collaborator

@dturner dturner left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. In memory datastore should also result in faster tests 👍

@dturner dturner merged commit ca2461f into android:main Oct 14, 2024
4 checks passed
@SimonMarquis SimonMarquis deleted the in-memory-datastore branch October 14, 2024 18:17
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.

4 participants