Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing #921

Merged
merged 7 commits into from
Mar 8, 2017
Merged

Testing #921

merged 7 commits into from
Mar 8, 2017

Conversation

yesmeck
Copy link
Member

@yesmeck yesmeck commented Mar 2, 2017

#900

  1. 原来的 npm test 改成了只用来跑测试;
  2. 新增 npm run test:web 跑 web 组件测试,npm run test:rn 跑 rn 组件测试;
  3. web 组件的测试用 .web.js 后缀;
  4. 测试写法可以参考 button 更多可以参考 antd 的测试

snapshot 的简单用法:

  1. http://facebook.github.io/jest/docs/tutorial-react-native.html#snapshot-test
  2. 如果 snapshot 有变更,可以 npm run test:[web/rn] -- -u 来自动更新 snapshot;
  3. 新增 demo 不要忘记跑一次测试把 snapshot 也提交上来;

碰到的问题/注意的点:

  1. native 组件只能用 react-test-renderer 来做 snapshot 的测试,而 web 的组件是用 enzymerender + enzyme-to-json 做的;
  2. native 组件只能用 enzymeshallow,不能用 mountrender

@yesmeck
Copy link
Member Author

yesmeck commented Mar 2, 2017

native demo 的 snapshot 测试还要研究下。

if (!this.props.disabled) {
this.setState({ pressIn: true });
}
if (this.props.onPressIn) {
this.props.onPressIn(arg);
(this.props.onPressIn as any)(...args);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里原来参数传错了,下面也一样。

@@ -102,7 +103,6 @@ export default class Button extends React.Component<tsProps, any> {
onShowUnderlay={this.onShowUnderlay}
onHideUnderlay={this.onHideUnderlay}
disabled={disabled}
{...restProps}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{...restProps} 写在下面会导致把上面的 onPressIn={this.onPressIn} 等覆盖掉。

Copy link
Contributor

@silentcloud silentcloud Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restprops 要把 onpressXX 过滤掉, 我来处理

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silentcloud OK,你改完 master 我再 rebase。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silentcloud 这个 master 上还没改掉。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个我来改吧

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

奥,已经改掉了。

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

本来想 web 组件和 RN 组件一起跑测试的,但是发现把 web 和 RN 环境混在一起有很多问题,现在的方案是分开来跑,引入两个命令 npm run test:webnpm run test:rn 但是这样做的话就导致测试覆盖率没法算了。各位有什么看法?

@afc163
Copy link
Member

afc163 commented Mar 7, 2017

平均一下发给 coveralls

不行,不只有总覆盖率。

@paranoidjk
Copy link
Contributor

  • 问题其实不在于两个覆盖率,如果能做到每个覆盖率的分母只包含对应的代码,就有意义,即 RN 的代码覆盖率计算的分母只包含RN的源码部分,web的是以web source code为分母计算

  • 真正代码运行的时候,还是分 RN 和 Web 两套环境的,所以如果能提供准确的 RN 和 Web 两个平台覆盖率指标也无不可,

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

决定分开来统计,test:web 只统计 .web.tsx 结尾的文件,test:rn 统计所有非 .web.tsx 结尾的文件。

@paranoidjk
Copy link
Contributor

这里有个坑是有些代码是复用的...

@paranoidjk
Copy link
Contributor

paranoidjk commented Mar 7, 2017

有些rn组件会引用.web代码,有些甚至不区分.web

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

有些rn组件会运用.web代码,有些甚至不区分.web

举个例子?

@paranoidjk
Copy link
Contributor

我手机不太方便,你可以找几个组件看一眼,真要区分估计要维护一份配置列表...这就有点尴尬了
cc ,@warmhug @silentcloud

@warmhug
Copy link
Contributor

warmhug commented Mar 7, 2017

底层依赖 picker date-picker 没区分文件名后缀,如 https://github.com/react-component/m-date-picker/blob/master/src/Popup.tsx cc @yiminghe

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

底层依赖没关系,这边的测试不测底层依赖的。

@yesmeck yesmeck force-pushed the testing branch 2 times, most recently from c9fd8a5 to eb5b2b7 Compare March 7, 2017 12:00
@paranoidjk
Copy link
Contributor

paranoidjk commented Mar 7, 2017

@yesmeck @warmhug Picker, PickerView, LocaleProvider 三个组件没加.web, RN和Web是复用的

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

这样的话,web 组件的覆盖率会偏高一点,但是 RN 组件的覆盖率是正常的。共享的代码在 web 测试里统计不到,在 RN 测试里会统计到。感觉关系不大。

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

也不能说 web 测试的覆盖率会偏高吧,只是说 web 测试的覆盖率会有少统计。

@paranoidjk
Copy link
Contributor

@warmhug 为了让覆盖率准确,Picker, PickerView, LocaleProvider这三个组件还是把文件拆开来ok不?类似index.web.tsx只是个空的,只 require index.tsx 并 export 出去

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

这样没用的,现在是按文件后缀名区分的。index.web.tsx 里只是一个 export 的话只算一行。

@paranoidjk
Copy link
Contributor

@yesmeck 哪尼? 代码覆盖率计算不会这样吧,Import的内容不算?

@yesmeck
Copy link
Member Author

yesmeck commented Mar 7, 2017

https://github.com/ant-design/ant-design-mobile/blob/testing/.jest.web.json#L26-L29

其实关系不大的,web 测试少统计的文件,在 RN 测试里会统计进去。覆盖率本来就是参考性的东西。

@paranoidjk
Copy link
Contributor

@yesmeck ok,优先把测试的流程run起来

@yesmeck yesmeck force-pushed the testing branch 3 times, most recently from 9d728dc to c4a174e Compare March 7, 2017 15:48
@codecov-io
Copy link

codecov-io commented Mar 8, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@f3c3d57). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #921   +/-   ##
=========================================
  Coverage          ?   60.71%           
=========================================
  Files             ?      259           
  Lines             ?     4612           
  Branches          ?     1299           
=========================================
  Hits              ?     2800           
  Misses            ?     1811           
  Partials          ?        1
Flag Coverage Δ
#rn 65.04% <ø> (?)
#web 54.67% <ø> (?)

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 f3c3d57...68043e8. Read the comment docs.

@yesmeck yesmeck force-pushed the testing branch 2 times, most recently from 2b73eb9 to cfbcc7a Compare March 8, 2017 05:19
@yesmeck
Copy link
Member Author

yesmeck commented Mar 8, 2017

Great! Codecov 能合并两个覆盖率报告。

@afc163
Copy link
Member

afc163 commented Mar 8, 2017

antd 也转 codecov 好了。

@yesmeck
Copy link
Member Author

yesmeck commented Mar 8, 2017

更新了描述,暂时把 snap 文件删掉了,ready to merge。

@yesmeck yesmeck changed the title [WIP]Introduce jest Testing Mar 8, 2017
@paranoidjk
Copy link
Contributor

@yesmeck 这个可以合了吗?

@yesmeck
Copy link
Member Author

yesmeck commented Mar 8, 2017

可以合并。

@paranoidjk
Copy link
Contributor

@yesmeck 7 个commit不需要合吗?

@yesmeck
Copy link
Member Author

yesmeck commented Mar 8, 2017

自动会合的。

@paranoidjk
Copy link
Contributor

@yesmeck 会保留你所有的commit log在注释里... 还是手动合一下吧

@yesmeck
Copy link
Member Author

yesmeck commented Mar 8, 2017

这不是很好吗。。把做了什么事都记下来。

@paranoidjk paranoidjk merged commit 5988bdb into master Mar 8, 2017
@paranoidjk paranoidjk deleted the testing branch March 8, 2017 14:23
@paranoidjk paranoidjk mentioned this pull request Mar 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants