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

Creates cycle indirection scenarios for tests #200

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gfreivasc
Copy link

Creates IndirectionCycle with regular provided dependencies,BindsIndirectionCycle for bound dependencies indirection, and MultibindProviderMapIndirectionCycle for the scenario pinpointed in issue #175 and resolved by #198.

@gfreivasc gfreivasc force-pushed the gabrielfv/staging/indirection-tests branch from a20b5e0 to a8a8415 Compare August 20, 2020 15:31
Creates `IndirectionCycle` with regular provided dependencies,
`BindsIndirectionCycle` for bound dependencies indirection, and
`MultibindProviderMapIndirectionCycle` for the scenario pinpointed in
issue JakeWharton#175 and resolved by JakeWharton#198.
@gfreivasc gfreivasc force-pushed the gabrielfv/staging/indirection-tests branch from a8a8415 to 7e778f4 Compare August 20, 2020 15:40
@TWiStErRob
Copy link
Contributor

There's a cycle test in upstream as well:

// TODO reflect-compiler bug! Not generating builder method.
exclude 'dagger/functional/builder/BuilderTest.java'
exclude 'dagger/functional/cycle/DoubleCheckCycleTest.java'

But I guess this will never execute because #4 (comment)

@TWiStErRob
Copy link
Contributor

What do you think about testing cycles and checking that the instances actually end up the same when scoped?

@gfreivasc
Copy link
Author

Quickly looking at it, it seems that perhaps some of these tests are okay to run. I'll take a look, since upstream currently covers two of the three tests.

@gfreivasc
Copy link
Author

I think the test scenario and tests suffice, right?

@gfreivasc gfreivasc force-pushed the gabrielfv/staging/indirection-tests branch from aa56017 to 80de396 Compare August 20, 2020 19:10
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