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

chore: Upgrade gradle 7.0 to 7.4 #1697

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

guqing
Copy link
Member

@guqing guqing commented Mar 2, 2022

@guqing guqing requested review from JohnNiang and ruibaby March 2, 2022 09:52
@guqing
Copy link
Member Author

guqing commented Mar 2, 2022

等待 #1691 合并后会修复CategoryServiceTest这个测试类,那时CI就能通过,没通过的原因是整体跑ci时OptionService查询的是否开启绝对路径值和单独跑CategoryServiceTest时不一致导致期望中链接地址不同,已在 https://github.com/halo-dev/halo/pull/1691/files#diff-c5c4429148eb87ced70e8e4020d5ce596bc914da8b466e3838e85e54969ad8b8 该文件重写测试

@JohnNiang
Copy link
Member

建议不要在同一个 PR 中做多件事情。这会增加 review 的复杂度。

值得一提的是,为什么升级 Gradle 会导致我们的测试出错。

@guqing
Copy link
Member Author

guqing commented Mar 2, 2022

建议不要在同一个 PR 中做多件事情。这会增加 review 的复杂度。

值得一提的是,为什么升级 Gradle 会导致我们的测试出错。

不晓得为什么isEnableFullPath的值会在不同情况下不一样 我提上面提到的那个pr跑整体测试的时候发现的 所以才修复了它 不是因为升级gradle导致的

@guqing
Copy link
Member Author

guqing commented Mar 3, 2022

建议不要在同一个 PR 中做多件事情。这会增加 review 的复杂度。
值得一提的是,为什么升级 Gradle 会导致我们的测试出错。

不晓得为什么isEnableFullPath的值会在不同情况下不一样 我提上面提到的那个pr跑整体测试的时候发现的 所以才修复了它 不是因为升级gradle导致的

@JohnNiang 经过定位原因如下:
https://github.com/halo-dev/halo/blob/master/src/test/java/run/halo/app/it/IndexPageRequestTest.java
该继承了


使用的是集成测试注解@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) @ActiveProfiles("test"), 并且在 indexPage 方法中调用了installBlog();方法其中
private void initSettings(InstallParam installParam) {

其中将启用绝对路径值由true改为了false

properties.put(OtherProperties.GLOBAL_ABSOLUTE_PATH_ENABLED, Boolean.FALSE.toString());

如果测试类的运行顺序改变先执行了IndexPageRequestTest这个测试类那么就会导致后执行的CategoryServiceTest的期望值不对,所以当我之前第一次发现这个问题时已经将CategoryServiceTest的测试方式重构了,后续避免测试类相互影响每个测试类都应该是独立的个体使用

@ExtendWith(SpringExtension.class)
// 或者
@ExtendWith(MockitoExtension.class)
// 来做并且事先对预期数据进行打桩,对测试主体进行mock
// 集成测试也要事先在前置条件准备预期环境已达到目的

为何测试类的顺序会改变?或许是添加了更多的测试类导致类字典顺序变了导致

Reference docs: https://docs.spring.io/spring-framework/docs/current/reference/html/testing.html#testcontext-framework

@JohnNiang
Copy link
Member

请注意,集成测试和单元测试不一样,甚至后面还会有 E2E 测试。

当然,非常感谢你能够排查测试错误原因。但是还有一个问题没有被挖掘到:为什么升级 gradle 之后,测试会失败?升级之前却没有任何问题。

@JohnNiang JohnNiang closed this Mar 3, 2022
@JohnNiang JohnNiang reopened this Mar 3, 2022
@JohnNiang
Copy link
Member

我将重新运行一下 CI,看看这是否为偶发情况。

@guqing
Copy link
Member Author

guqing commented Mar 3, 2022

请注意,集成测试和单元测试不一样,甚至后面还会有 E2E 测试。

当然,非常感谢你能够排查测试错误原因。但是还有一个问题没有被挖掘到:为什么升级 gradle 之后,测试会失败?升级之前却没有任何问题。

不是升级gradle这个才失败的,而是#1691 这个PR首次被发现,所以这个PR我改了CategoryServiceTest的测试方式。
我知道集成测试和单元测试是不一样的,我上面是说尽量在对整个类测试时是针对性的对类测试,如果是集成测试也要准备好前置条件 每个类都是整体不应该影响其他类,除非是e2e测试

@JohnNiang JohnNiang closed this Mar 3, 2022
@JohnNiang JohnNiang reopened this Mar 3, 2022
Copy link
Member

@JohnNiang JohnNiang left a comment

Choose a reason for hiding this comment

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

LGTM

@JohnNiang JohnNiang merged commit 0f33ee1 into halo-dev:master Mar 3, 2022
@guqing guqing deleted the feature/upgrade-gradle branch March 3, 2022 03:25
winar-jin pushed a commit to winar-jin/halo that referenced this pull request Mar 24, 2022
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.

2 participants