Remove style guide and use relative paths for tests #101
Conversation
tajo
requested review from
hturan,
sejoker,
wyuenho,
toekneestuck,
jculvey,
terinjokes,
koddsson,
marksteyn,
prestonp,
nkcmr,
manatarms and
jwineman
|
-4k lines, that's what I like to see! Good stuff. So is the plan here to make our cf-ui documentation private? I'm kind of confused around what |
package.json
Outdated
| "test": "cross-env BABEL_ENV=commonjs jest", | ||
| "updated": "lerna updated --only-explicit-updates", | ||
| "prettier": "node prettier.js", | ||
| "clean": "lerna clean --yes && rimraf .jest dist", | ||
| "clean": "lerna clean && rimraf dist", |
There was a problem hiding this comment.
rimraf .jest needs to be part of npm run clean to fix the jest cache issues we were having earlier this week.
There was a problem hiding this comment.
also I don't know what --yes does but did you remove it on purpose?
There was a problem hiding this comment.
Good catch. My mistake during the rebase.
|
|
||
| test('.flashRow() should dispatch actions for flashing the row', done => { | ||
| // this test is flaky, in 1/4 cases it just time outs | ||
| test.skip('.flashRow() should dispatch actions for flashing the row', done => { |
There was a problem hiding this comment.
I run npm test -- --testPathPattern cf-builder-table 10 times without flaky results. Let's hold on with skiping it
There was a problem hiding this comment.
Yea, it seems ok now. Fixed.
|
I am ok with changing pathes to relative, but as @hturan mentioned moving styleguide into private repo is something I want to discuss before making a change. It quite important to have open styleguide and within same repo, much easier to test. |
|
@hturan @sejoker I'm going to document everything today. The reason why we are removing the styleguide from this repo is to streamline its development. We need to use it for our private styles and private components as well and we don't want to maintain two different versions. It's pretty hard to have a general styleguide app that could be open sourced right now (in sense of making no assumptions about our internal projects). The priority now is to provide the best DX and seamless integration with cf-ui-private and cf-www-next without running some crazy .sh scripts. That doesn't mean we can't bring it back to public later.
I agree that the styleguide is critical for deving and testing of cf-ui/cf-ui-private components. It's one of its primary goals - to help developers maintain our React components. However, it doesn't have to be in the same repo thanks to some webpack magic. ^^ We were discussing this with @jculvey @jwineman and @toekneestuck. |
| @@ -101,6 +110,6 @@ test('should support flashes', () => { | |||
| </Provider> | |||
| ); | |||
| store.dispatch(tableActions.flashRow('test-table', '1', 'success')); | |||
| const row = wrapper.find(TableBody).find(TableRow).at(0); | |||
| expect(row.prop('type')).toBe('success'); | |||
| const row = wrapper.find('tbody').find('tr').at(0); | |||
There was a problem hiding this comment.
Enzyme was giving me this error:
Method “props” is only meant to be run on a single node. 0 found instead.
Not sure why so I'm testing against the markup.
| @@ -86,7 +85,7 @@ test('should should call onPageChange when clicking another page', () => { | |||
| /> | |||
| ); | |||
|
|
|||
| const items = wrapper.find(PaginationItem); | |||
| const items = wrapper.find('li'); | |||
There was a problem hiding this comment.
Explained above. Not sure why Enzyme doesn't like this anymore but the test is still legit (testing the same functionality).
Another 100+ files PR but the vast majority of changes is just a replacement of absolute paths in the tests. I was using babel to remap all packages but it turns out that it is a disaster if you start "touching" this repo from other repos.
That's it. Please LGTM asap.
I'm going to document cf-styleguide since it uses a lot of webpack magic and write a private blog post as well.