Add Tests for components #3486
Comments
alitaheri
added
umbrella
|
Since testing is an ongoing activity, my recommendation for this issue is that maybe we shoot for a general percentage of coverage for |
|
@alitaheri BTW, the I don't want people to misinterpret the checked boxes as meaning, "this is perfectly tested", because at this stage the important thing is to get some basic test coverage on all the components rather than having perfect tests for half of them. |
Yup, these are my thoughts exactly. Technically this task is never finished, but setting some internal goal would be good for all the components. |
|
@newoga @alitaheri What do you want to test in terms of styling? Additionally, just a side not, but it's important not to confuse 100% coverage with 100% bulletproof ;) |
I agree, but it the numbers definitely help keep us accountable. 😄 |
|
And ensure that the components actually do something 💃 |
|
I will have more time after this week btw to contribute more, just one of those weeks at the office ;) |
|
That's great, thanks much @nathanmarks! I've having one of those weeks as well. |
|
By styles I mostly refer to transformers working as expected. I'll try to come up with something to reflect goal coverage 😁 |
|
Awesome! PS, is there a convention in place for the naming of exporting the plain components for those that have an HoC wrapping the default export? |
|
Not really, but it would be great if there was. How about exporting as |
|
That was my initial thought, but I was wondering if it makes it too easy to export the wrong component by accident. Anyone else? @newoga @oliviertassinari? import Component from 'material-ui/lib/Component';
import { Component } from 'material-ui/lib/Component'; |
|
Yeah... import Component from 'material-ui/lib/Component';
import { Raw as Component } from 'material-ui/lib/Component';Might be harder to get wrong, right? wrong? |
|
700% harder to get wrong. |
|
Yeah I need to give this more thought. I haven't thought of something I'm completely happy with. Though maybe it will make sense when we reorganize. import Component from 'material-ui/lib/Component';
import { Component } from 'material-ui/lib/Component';I think the approach above shouldn't return differently, that could be confusing. import Component from 'material-ui/lib/Component';
import { Raw as Component } from 'material-ui/lib/Component';I think this is more explicit and much safer. import Component from 'material-ui/lib/Component';
import RawComponent from 'material-ui/lib/Component/raw';Just another idea. |
|
I think the |
|
I think if I had to pick one now, I'd go with: import Component from 'material-ui/lib/Component';
import { Raw as Component } from 'material-ui/lib/Component';Though I'm wondering raw isn't the right word. |
|
That's the truth. Out of curiosity, do we plan on supporting the exports of raw components to developers? While I do see some interesting use cases when that could be useful, I'd rather not make it available publically unless we were really sure we are ready to support it. Even if the raw exports are not documented, people will find out and use them anyway. I'd rather not have to worry about deprecating old raw named exports and paths if we ever decide we want to change or reorganize it. Javascript isn't the best at helping us in this area. For example, see what Facebook had to do in the React source: https://github.com/facebook/react/blob/master/src/React.js#L25 I suppose the biggest difference in this case is that we only need the raw components for testing. So if we were clever, there might be a way to remove the exports when we do an actual release build. If the exports were removed, I'd be completely fine naming the exports raw or anything and not feeling bad if we need to switch it later. Let me know if I'm being too paranoid. 😆 |
|
I think that they should stay undocumented. Stick I also feel weird naming them all FWIW, in my own projects I use |
|
|
|
I was having visions of |
😆 😆 😆 |
|
Poll needed? LOL! |
|
👍 for |
|
@alitaheri Just to confirm, by that are you thinking along the lines of |
|
Yes I was also referring to the pattern |
|
Sounds good to me too! |
|
Are there any components we should hold off on unit testing that are scheduled for heavy refactoring? |
All of them. 😁 |
|
@mbrookes 😄 hahaha. Let me refine my question then. There are some components with questionable behaviour (table composition issues spring to mind) that I imagine might require heavy refactoring to change. Therefore, creating tests in it's current state feels like asserting undesirable behaviour (and a semi-waste of time). For example, trying to add any click handlers except on the root |
|
I'd have to look over everything again with a fresh perspective. I think some component APIs are closer than others, and some will likely require a complete redesign. The challenge is I don't think we're going to have anytime to do any of the major component API refactoring in the That being said, I think the two that came immediately to mind that I'd like to remove are |
|
What do you mean by "new components"? |
|
Sorry, that was confusing. I meant existing components that don't currently have tests. |
|
If that's the case, is there anything you can advise me to definitely not write unit tests for at this time? |
|
I just noticed the two I mentioned before @alitaheri didn't even bother including on the list. Honestly other than that I'm not too sure. I'd love to see rewrites for one or two components, but it's just not on my radar at the moment. My recommendation is if you start looking at a component that needs to be tested and you're seeing some major design flaws that maybe aren't worth testing and should be just fixed, let's discuss it on Gitter or Github and plan what we should do with it. |
|
@newoga To continue the discussion from #3248 I think that the important thing, regardless of |
react-look is a game changer. we would probably move a lot of logic there. I plan on adding memoization to react-look, so we can have a single call to prepareStyles with each change of inputs. it's going to be all inside the react-look. but maybe we can test the functions that take theme, state, props, etc and see if they return some expected styles. |
|
@alitaheri since this isn't being updated we may want to get rid of that check list and close the issue. |
|
Or start updating it. |
After the great work done by @nathanmarks with the tests we should now go ahead and make unit tests for each component to ensure their integrity 👍
This is the list of all components that need unit tests:
Components:
app-bar: [Tests] Add unit tests for AppBar #3487 @pradelauto-completeavatar: Partial: ([tests] Updates to test setup and additional testing option for unit tests #3405 @nathanmarks)badge: [Tests] Badge unit tests #3427 @pradelbuttons/flat-button-labelDrawercard/cardcard/card-actionscard/card-expandablecard/card-headercard/card-mediacard/card-textcard/card-titlecheckboxcircular-progressdate-picker/calendardate-picker/calendar-monthdate-picker/calendar-toolbardate-picker/calendar-yeardate-picker/date-displaydate-picker/date-pickerdate-picker/date-picker-dialogdate-picker/date-picker-inlinedate-picker/day-buttondate-picker/year-buttondialogdividerDropDownMenu/DropDownMenuenhanced-buttonenhanced-switchenhanced-textareaflat-buttonfloating-action-buttonfont-icon: [Tests] Add unit tests for FontIcon #3490 @pradelgrid-list/grid-list: [Tests] Add unit tests for GridList #3488 @pradelgrid-list/grid-tileicon-buttonink-barlinear-progresslists/listlists/list-itemlists/nested-listmenus/icon-menumenus/menumenus/menu-itemoverlaypaperpopover/popoverpopover/popover-animation-from-toppopover/popover-default-animationradio-buttonradio-button-groupraised-buttonrefresh-indicatorrender-to-layerripples/circle-rippleripples/focus-rippleripples/touch-rippleSelectField/SelectFieldslidersnackbarSubheader/Subheadersvg-icon: [Tests] Add unit tests for SvgIcon #3489 @pradeltable/tabletable/table-bodytable/table-footertable/table-headertable/table-header-columntable/table-rowtable/table-row-columntabs/tabtabs/tabstabs/tabTemplateTextField/TextFieldTextField/TextFieldHintTextField/TextFieldLabelTextField/TextFieldUnderlinetime-picker/clocktime-picker/clock-hourstime-picker/clock-minutestime-picker/clock-numbertime-picker/clock-pointertime-picker/time-displaytime-picker/time-pickertime-picker/time-picker-dialogtoggletoolbar/toolbartoolbar/toolbar-grouptoolbar/toolbar-separatortoolbar/toolbar-titletooltipUtility Components:
ClickAwayListenermuiThemeableMuiThemeProvidertransition-groups/scale-intransition-groups/scale-in-childtransition-groups/slide-intransition-groups/slide-in-childStyles:
styles/getMuiTheme.jsstyles/transformers/autoprefixer.jsstyles/transformers/rtl.jsstyles/transitions.jsThe text was updated successfully, but these errors were encountered: