Class Initialize inheritance #143 #577
Conversation
…ue143-classinitializeinh
…ue143-classinitializeinh
|
cc: @AbhitejJohn @jayaranigarg @singhsarab |
|
Pushed new changes to fix that base cleanup were being called without calling base initialize |
There was a problem hiding this comment.
Thanks for putting this out. Added my feedback. @acesiddhu or @jayaranigarg would be able to address the framework attribute concern better but looks good overall.
…ue143-classinitializeinh
|
@dotnet-bot test Windows / Release Build please |
…ue143-classinitializeinh
…ainc/testfx into issue143-classinitializeinh
|
So when can we expect this to be merged? I really need to be able to use [ClassInitialize] in my custom base class and if I'm correct this will make that possible. Right? |
…ue143-classinitializeinh
…ainc/testfx into issue143-classinitializeinh
From my side, I can tell you that since I've been kind of busy, i'm working a little bit slower than before on this, but even though, I think by end of next week I'll be done with this (unless there's more review to solve). This changes will let you tell the framework to take into account your custom base class' initialize method when running your tests. So, yeah, you're correct. I think @jayaranigarg and @AbhitejJohn could expand a bit more when this would be released after merged into master. |
There was a problem hiding this comment.
Thanks for updating the logic. It might need a few minor touch-ups and testing but apart from the comments I've put in, it looks good. I'm hoping @jayaranigarg or @cltshivash would be able to help with approvals and merging. I'm just helping as a fellow open source-rer and cannot merge this in myself. I can help with the cleanups if I need to, to get this in quicker.
| initializeMethod = baseClassInitializeStack.Pop().Item1; | ||
| if (initializeMethod != null) | ||
| { | ||
| initializeMethod?.InvokeAsSynchronousTask(null, testContext); |
There was a problem hiding this comment.
We do not need this null check operator here given that we are already performing a null check above.
There was a problem hiding this comment.
actually, i think we might needed it since the null check above is to make sure there are any non-null init method in the tuple that can be executed. In any case, I changed this for a .Where call when newing up the baseClassInitializeStack (on the this.BaseClassInitAndCleanupMethods) that way, we just pass executable init methods, and we don't need to do any extra thing to pass its corresponding cleanup method to the stack for cleanup methods to execute in the RunClassCleanup method.
There was a problem hiding this comment.
It looks like the updated code wouldn't run Cleanups from base classes that do not have an initialize method...
| while (baseClassCleanupQueue.Count > 0) | ||
| { | ||
| classCleanupMethod = baseClassCleanupQueue.Dequeue().Item2; | ||
| classCleanupMethod?.InvokeAsSynchronousTask(null); |
There was a problem hiding this comment.
shouldn't we be only executing cleanups whose inits have run? This seems to be running all cleanups.
There was a problem hiding this comment.
Maybe we could just push Cleanups that need to be executed into a Stack instead of ExecutedBaseClassInitializeMethods
There was a problem hiding this comment.
Yup, I had a block of code that I removed right before pushing the latest changes, handling the logic for deciding those cleanup method that will be executed. But, now that you mentioned that last part, I think that'd be better. For that, renamed the property you mentioned, now it's called: BaseClassCleanupMethodsStack not sure if the name is 100% ok, and in the RunClassInitialize method, I'll be pushing the cleanups into that stack to then execute them in the RunClassCleanup method.
There was a problem hiding this comment.
Sounds good. It doesn't look like we need a Queue here. We could just pop the methods off the stack..
|
Hi Do you know when this change will be merged and released? Cheers Paul |
|
I'd like to see this in too. |
|
@parrainc Any updates on this change? Look like there hasn't been progress for a bit. I'm in need of this and would be happy to help if needed. |
|
So, I've checked offline with the team on getting this in. The plan is to do the needful to merge this in early next week. I believe @parrainc has addressed most of the feedback here and I'd love to have this in the product. |
|
I think most of the comments (if not, all of them) were addressed. I've been a little busy but if something still missing I could fix it, so it could be merged. So, basically, I'm waiting on @jayaranigarg @cltshivash inputs. If this looks good for them as well, then we should be good. |
|
@sturlath @paulmccormack @NGloreous You can consume this change using following nuget packet versions: |
|
@parrainc must classinitialize be static for it to run? i do not have it marked static and it will not run from a base class |
Implemented feature requested in #143
example output:
===> Init from ParentTestClass
==> Init from BaseTestClass
=> Init from DerivedTestClass
=> Cleaning up from DerivedTestClass
==> Cleaning up from BaseTestClass
===> Cleaning up from ParentTestClass