search

OrleansContrib / OrleansTestKit

Unit Test Toolkit for Microsoft Orleans
http://dotnet.github.io/orleans
MIT License
78 stars 43 forks source link

Resolve the GrainType from the provided generic grain value #141

Closed Romanx closed 1 year ago

Romanx commented 1 year ago

closes #140

Romanx commented 1 year ago

Hey @seniorquico,

Sorry it was late on Friday and didn't give a good description. This corrects the GrainType on the created grains to be the same as Orleans so anything that is looking at the Type value, including any aliases, are correct. I can add a test for that specific case.

We had some code that looks the GrainType and was failing since it was an invalid value.

I used a DI container since the GrainTypeResolver has an interesting dependency tree and this approach handles Orleans changing how this value is generated since it's extensible. I could make it more explicit but there might have to be manual changes if Orleans changes functionality.

I'd rather not be creating the container and resolving the value each Silo but TestKit seems to isolate itself on that level so it seemed sensible. It could be made private and static so it's only resolved since it shouldn't change but didn't want to assume.

Romanx commented 1 year ago

@seniorquico Sorry for the delay, added tests showing what this change provides.

Romanx commented 1 year ago

@seniorquico Any thoughts on this? It's a blocker for using testkit due to the GrainType and I can't really use reflection to hack around it since most of the other pieces in CreateGrainAsync are internal so i can't duplicate it locally.

Romanx commented 1 year ago

@seniorquico Another gentle nudge on this since all our tests are broken without this change and theres no way for me to hack around it so it's blocking us. Happy to make more changes if you'd like

Romanx commented 1 year ago

@seniorquico Another bump on this. Happy to change approach with some feedback.

Looked at the latest changes with grain context. It gets me some way to being able to do this externally to testkit but even if I override the GrainContext with the "correct" grain id with the grain type correctly, the context gets replaced since the comparison is done on the whole grain id.

If you'd rather not the service provider route so the grain types are correct for all grains I could instead expose an overload of GetOrAddGrainContext that takes a whole GrainId and allows you to set that?

fergusbown commented 1 year ago

HI, sorry to keep prodding, I work with @Romanx - any chance of progressing this, we could really do with it :) Thanks!