Skip to content

test: add regression tests for FileConfiguration - #8151

Open
amirdeljouyi wants to merge 6 commits into
apache:2.xfrom
amirdeljouyi:test-add-regression-tests-for-fileconfiguration
Open

test: add regression tests for FileConfiguration#8151
amirdeljouyi wants to merge 6 commits into
apache:2.xfrom
amirdeljouyi:test-add-regression-tests-for-fileconfiguration

Conversation

@amirdeljouyi

@amirdeljouyi amirdeljouyi commented Jun 28, 2026

Copy link
Copy Markdown

test: add regression tests for FileConfiguration

Ⅰ. Describe what this PR did

I added three regression tests for the file-backed configuration API.

The tests cover reads, default-value behavior for missing keys, and mutation operation outcomes.

Impact on coverage:

The tests cover the main file-backed read and mutation paths in FileConfiguration, plus timeout handling in the config operation runnable. In the focused JaCoCo
run, FileConfiguration.java branch coverage increases around 3%.

Ⅱ. Does this pull request fix one issue?

No linked issue; this is a test-only regression-coverage change.

Ⅲ. Why don't you add test cases (unit test/integration test)?

This PR consists entirely of unit tests.

Ⅳ. Describe how to verify it

Run ./mvnw -pl config/seata-config-core -am -Dtest=FileConfigurationTest -DfailIfNoTests=false test.

Ⅴ. Special notes for reviews

No production code or API behavior is changed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds additional unit test coverage around the file-backed Configuration implementation accessed via ConfigurationFactory, aiming to prevent regressions in read/default/mutation paths.

Changes:

  • Adds three new regression tests validating file-backed reads, default behavior for missing keys, and mutation outcomes through ConfigurationFactory.
  • Refactors existing assertions to use static assertTrue/assertFalse imports.
  • Updates many test methods to declare throws Exception.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@WangzJi WangzJi added the type: test test case label Jul 10, 2026
@amirdeljouyi
amirdeljouyi force-pushed the test-add-regression-tests-for-fileconfiguration branch from b27a25b to c06688b Compare July 12, 2026 13:43
@WangzJi
WangzJi requested a review from Copilot July 13, 2026 02:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.06%. Comparing base (f97462e) to head (a96f5af).

Additional details and impacted files
@@             Coverage Diff              @@
##                2.x    #8151      +/-   ##
============================================
+ Coverage     72.99%   73.06%   +0.06%     
  Complexity     1141     1141              
============================================
  Files          1151     1151              
  Lines         42272    42272              
  Branches       5045     5045              
============================================
+ Hits          30856    30884      +28     
+ Misses         8938     8915      -23     
+ Partials       2478     2473       -5     

see 7 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@WangzJi

WangzJi commented Jul 25, 2026

Copy link
Copy Markdown
Member

Could you please add the pr changes?

@amirdeljouyi

Copy link
Copy Markdown
Author

Hi @WangzJi, I added the PR change entry to changes/en-us/2.x.md and updated the checklist. Thanks.

…tests-for-fileconfiguration

# Conflicts:
#	changes/en-us/2.x.md

@slievrly slievrly left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

中文 / Chinese

纯测试 PR,只给 FileConfiguration 补回归覆盖,不动任何生产代码 —— 方向和范围都清晰,值得合并。三个用例覆盖了读、缺省值、mutation 结果三条主路径,System property 也做了保存/恢复,整体质量不错。

几点建议(非阻塞):

1. shouldReportMutationOperationOutcomeThroughConfigurationFactory 的断言语义偏弱

putConfig / putConfigIfAbsent / removeConfig 只断言了返回值 true/false,没有验证操作实际生效(比如 put 之后 getConfig 能读到、remove 之后读不到)。file 配置的 mutation 本质是异步 + listener 驱动的,只测返回值容易变成"接口没抛异常"级别的覆盖。建议至少对 put 后补一个读回断言。

2. 负数 timeout 走 false 分支 —— 建议加注释说明意图

putConfig("...", "value", -1L) 返回 false 依赖的是"超时立即到期"这条内部路径。-1L 这个魔法值对后来读代码的人不直观,加一行注释("negative timeout forces immediate expiry, exercising the timeout branch")会更友好。

3. shouldDelegateFileBackedReadsThroughConfigurationFactory 依赖测试资源里的固定值

断言 127.0.0.1:8091disableGlobalTransaction=false 来自测试 classpath 的配置文件。如果哪天有人改了那个 fixture,这个测试会莫名其妙挂掉,且报错信息看不出根因。建议注释里点明这两个期望值来自哪个 file.conf / registry.conf

4. 与既有 FileConfigurationTest 的重复度

建议扫一眼同文件里已有的用例,确认新增的三个没有和现存覆盖大量重叠(尤其 getConfig 缺省值那类)。test-only PR 的价值在于补真正的空白,不在于堆数量。

整体 LGTM,上面都是 nice-to-have。


English

Test-only PR adding regression coverage for FileConfiguration, no production code touched — clear scope, worth merging. The three cases cover reads, defaults, and mutation outcomes, and system properties are saved/restored properly. Good quality overall.

A few non-blocking suggestions:

1. Weak assertion semantics in shouldReportMutationOperationOutcomeThroughConfigurationFactory

putConfig / putConfigIfAbsent / removeConfig only assert the boolean return, not that the operation actually took effect (e.g. getConfig reads the value back after put, or returns null after remove). File-backed mutation is async + listener-driven, so asserting only the return value risks being "the method didn't throw"-level coverage. Suggest adding a read-back assertion after put.

2. Negative timeout hits the false branch — add a comment on intent

putConfig("...", "value", -1L) returning false relies on the internal "timeout already expired" path. The -1L magic value isn't obvious to a future reader; a one-line comment ("negative timeout forces immediate expiry, exercising the timeout branch") would help.

3. shouldDelegateFileBackedReadsThroughConfigurationFactory depends on fixed test-resource values

Asserting 127.0.0.1:8091 and disableGlobalTransaction=false couples the test to a config file on the test classpath. If someone changes that fixture later, this test breaks with an opaque failure. Suggest noting in a comment which file.conf / registry.conf these expected values come from.

4. Overlap with existing FileConfigurationTest

Worth scanning the existing cases in the same file to confirm the three new ones don't heavily overlap existing coverage (especially the default-value getConfig cases). The value of a test-only PR is filling genuine gaps, not adding count.

LGTM overall — the above are all nice-to-haves.

@amirdeljouyi

amirdeljouyi commented Jul 30, 2026

Copy link
Copy Markdown
Author

Hi @slievrly, thanks for the review. I added comments clarifying the fixture values, current mutation behavior, and negative timeout. I left out the read-back assertion because file-backed mutations currently do not rewrite the source file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: test test case

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants