Skip to content
This repository has been archived by the owner on Aug 26, 2021. It is now read-only.

sorting fields for deterministic order #565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

contextshuffling
Copy link

When using reflection to get the fields from a class, the order of the fields is not guaranteed to be in any specific order. Some tests, like multiValueBindings_WithSingletonsAcrossMultipleInjectableTypes, may fail due to a different ordering returned by getDeclaredFields. This PR sorts the field array returned by getDeclaredFields to guarantee a deterministic order (in this PR, alphabetical order).

Please let me know if you want to discuss the changes more.

@JakeWharton
Copy link
Member

It's not clear how that test would fail as each type only contains a single field. This is an expensive operation that should have no practical effect on real code. We should correct the test failures, if any.

Also, Dagger 1 isn't being worked on and almost certainly won't see any future release.

@contextshuffling
Copy link
Author

Hi @JakeWharton ,

Sorry for the late response. The test in question is actually multiValueBindings_WithSingletonAndDefaultValues(), instead of the one mentioned in the original post, sorry about that.

This test prepares a class that has two fields. When getDeclaredFields() is eventually called using this class, the fields are not guaranteed by the API to come in a specific order, so it is possible for the order to flip. As such, the test can actually fail since right now it assumes a specific order. If during injection, the field that gets injected first will contain (100, 200), and the one gets injected second will contain {100, 201}. But as mentioned, it is not guaranteed that objects1 always gets injected first and thus, the test may fail when objects2 gets injected first.

While sorting the fields is what we propose in this PR, we understand that the operation is expensive. However, we would still like to make the test more robust. Would a valid solution be to assert that both scenarios are possible, so check if the values are one way or the other?

@JakeWharton
Copy link
Member

I would prefer that, yes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants