Codecov Report

Merging #8757 into master will decrease coverage by 0.02%.
The diff coverage is 22.22%.

@@            Coverage Diff            @@
##           master   #8757      +/-   ##
=========================================
- Coverage   58.82%   58.8%   -0.03%     
=========================================
  Files         526     526              
  Lines       24563   24580      +17     
  Branches     3974    3976       +2     
=========================================
+ Hits        14450   14454       +4     
- Misses       9178    9191      +13     
  Partials      935     935

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c5b2fd...89a9fa5. Read the comment docs.

While trying to write a unittest for the added picker option i stumbled over something in picker.unit.test.ts. In this line an object is build whose value property should be of type CommandSource but gets a Type object as input. Is this intentional? Because the resulting combinations do not make sense to me. You get different types of commands invoked from different types and not different types invoked from different command sources... Don Jayamanne (@DonJayamanne) could you clarify that?

In terms of how it looks, it's great :) No clue about the code part though 😝
Also, Don is now working on the data science features, so you may want to ask those questions to Karthik Nadig (@karthiknadig) instead 😊

Karthik Nadig (@karthiknadig) as Luciana Abud (@luabud) told me, that you can also look at the changes, what do you think?

Philipp Loose (@phloose) I am taking a look at this. For the unit tests you are right, it should be passing CommandSource and it passes Type instead.

Philipp Loose (@phloose) let me know if you plan on updating the tests.

Philipp Loose (@phloose) let me know if you plan on updating the tests.

What do you mean exactly? Should i enhance the existing ones? Or test also the used TestDisplay methods? I realized that there are no tests that test TestDisplay methods...

I thought i wait for your feedback and if you are concerned about something i would refactor the code to your suggestions or add parts i missed/didn't think of.

Philipp Loose (@phloose) Sorry, I thought you were waiting to do the Type to CommandSource change. I missed that. These changes look good.

Philipp Loose (@phloose) Sorry, I thought you were waiting to do the Type to CommandSource change. I missed that. These changes look good.

Actually i already made the change in 98f3316 ;)

Thanks for your approval! I think in general that there should be more tests for the TestDisplay class which does the real work. But in #8647 i already developed a small test case for that class and will enhance it to be a base for future tests for TestDisplay. If i would add something here i would do double work and in the end there would be conflicts. Maybe there should be a follow up issue to this pull-request that adresses writing proper tests for the additions to TestDisplay made here.

The smoke test failure is unrelated to this PR.

Philipp Loose (@phloose) We are beginning to address test gaps in the following sprints. I agree, we need tests covering TestDisplay. We are using analysis tools to find gaps and address them. If you are planning on addressing TestDisplay or are already working on it, please create a issue for it. You could submit a draft PR, so we know you are looking at it, and we can prevent overlaps.