Add menu item to Test Explorer context menu allowing Ignore/Unignore of unit test #5064
Conversation
IvenBach
added
the
PR-Status: WIP
There was a problem hiding this comment.
There are multiple ways to handle visibility. If we’re going to use two separate VM props for the different menu items, and just toggle visibility, I would stick to the way you did it.
Alternatively, there could be a single menu item whose text and command get bound to single VM props. I personally think either is a fine solution.
Where are the tests for the new code though?
Context menu for TestExplorer has a command enabled for Ignoring/Unignore that will add/remove the annotation.
Replaced conditional property that would change with explicit properties. Forgot to remove when `UnignoreTestLabel` and `IgnoreTestLabel` replaced it.
IvenBach
force-pushed
the
Issue2964_TestExplorerDisableTestViaContextMenu
branch
from
789cfa4
to
86dbaf5
Compare
| public string UnignoreTestLabel => RubberduckUI.TestExplorer_TestToggle_Unignore; | ||
| public string IgnoreTestLabel => RubberduckUI.TestExplorer_TestToggle_Ignore; | ||
|
|
||
| private TestMethod _mousedOverTestMethod => ((TestMethodViewModel)SelectedItem).Method; |
There was a problem hiding this comment.
if we check for SelectedItem being null, this shouldn't assume it is not. The reason for that is that we can't ensure this is only evaluated in Display(Uni|I)gnoreTestLabel. It might be easier to have something like SelectedItemIgnoreMatches(bool ignoreState) => (((TestMethodViewModel)SelectedItem)?.Method.IsIgnored ?? !ignoreState) == ignoreState;. This is a bit... dense, though, so a proper method would probably be best ;)
There was a problem hiding this comment.
I'll need more breadcrumbs to follow and understand this.
There was a problem hiding this comment.
I wonder whether it would make sense to have some toggle ToggleIgnoreTestCommand to be property injected into the view model. However, that might complicate things a little.
Anyway, I would suggest to actually do DI all the way and inject the IAnnotationUpdater.
| @@ -14,7 +14,7 @@ public MockedTestExplorer(MockedTestExplorerModel model) | |||
| Vbe = model.Engine.Vbe.Object; | |||
| State = model.Engine.ParserState; | |||
| Model = model.Model; | |||
| ViewModel = new TestExplorerViewModel(null, Model, ClipboardWriter.Object, null, null); | |||
| ViewModel = new TestExplorerViewModel(null, Model, ClipboardWriter.Object, null, null, null, null); | |||
There was a problem hiding this comment.
As a general rule, we should avoid newing things in tests but rather reference to a single static method Arranage.... That helps us avoid having a ugly refactoring session when we need to modify the parameters for the ctor. This puppy is already up to 7 parameters 😨 so it's surely itching to be refactored.
|
https://github.com/IvenBach/Rubberduck/blob/Issue2964_TestExplorerDisableTestViaContextMenu/Rubberduck.Core/UI/UnitTesting/TestExplorerControl.xaml#L421 for not-so-distant-future-Iven to attempt to add |
Close #2964.
TODO:
ExecuteToggleTestAnnotationCommandneeds some love).IsSelectionATestis a lie. Make it check what it's claiming.Question:
TestExplorerControlappropriate? I thought about having it be a property on the VM but that didn't feel right when I was trying to code it up.