Aias00 commented on PR #7721:
URL: https://github.com/apache/shenyu/pull/7721#issuecomment-6258141911
Heads-up: master is currently red because two assertions added in this PR
contradict the instance-info contract that #7785 established an hour earlier.
## Failure
`test_group (admin-sync, shenyu-admin, ...)` on master (`de93952d`) fails
with:
```
InstanceCheckServiceTest.testHandleBeatInfoRefreshesInstanceInfoOnExistingIdentity:178
expected: <version=1.0> but was: <{}>
InstanceCheckServiceTest.testHandleBeatInfoKeepsInstanceInfoWhenBeatCarriesBlank:198
expected: <version=1.0> but was: <{}>
```
This is not a flake and it is not caused by #7749 — that PR simply inherits
the same two failures (it also runs 7 additional tests of its own, all of which
pass).
## Root cause
#7785 (merged as `7086afaf`) changed the insert path in
`InstanceCheckService.handleBeatInfo` to normalize what the beat reports:
```java
instanceInfoVO.setInstanceInfo(InstanceInfoUtils.normalize(instanceBeatInfoDTO.getInstanceInfo()));
```
and pinned the contract in
`testInvalidOptionalInformationDoesNotStopHeartbeats`:
```java
@ValueSource(strings = {"{broken}", "[]", "true", "not-json"})
...
assertEquals("{}", actual.getInstanceInfo());
```
`"version=1.0"` in the two tests added here is exactly `not-json`, so
`normalize` returns `{}` and both assertions fail. The two contracts cannot
both hold.
## Requested change
Please use a JSON object payload in the two tests, which keeps their actual
intent (metadata is refreshed on repeated heartbeats) while conforming to the
storage contract:
```java
@Test
void testHandleBeatInfoRefreshesInstanceInfoOnExistingIdentity() {
InstanceBeatInfoDTO first = buildDTO("127.0.0.1", "8080", "grpc", "ns");
first.setInstanceInfo("{\"version\":\"1.0\"}");
instanceCheckService.handleBeatInfo(first);
InstanceInfoVO cached =
instanceCheckService.getInstanceHealthBeatInfo(first);
assertNotNull(cached);
assertEquals("{\"version\":\"1.0\"}", cached.getInstanceInfo());
InstanceBeatInfoDTO second = buildDTO("127.0.0.1", "8080", "grpc", "ns");
second.setInstanceInfo("{\"version\":\"2.0\"}");
instanceCheckService.handleBeatInfo(second);
assertEquals("{\"version\":\"2.0\"}", cached.getInstanceInfo());
}
@Test
void testHandleBeatInfoKeepsInstanceInfoWhenBeatCarriesBlank() {
InstanceBeatInfoDTO first = buildDTO("127.0.0.1", "8080", "grpc", "ns");
first.setInstanceInfo("{\"version\":\"1.0\"}");
instanceCheckService.handleBeatInfo(first);
InstanceInfoVO cached =
instanceCheckService.getInstanceHealthBeatInfo(first);
assertNotNull(cached);
InstanceBeatInfoDTO blank = buildDTO("127.0.0.1", "8080", "grpc", "ns");
blank.setInstanceInfo("");
instanceCheckService.handleBeatInfo(blank);
assertEquals("{\"version\":\"1.0\"}", cached.getInstanceInfo());
}
```
`normalize` returns the canonical form unchanged for an already-compact JSON
object, so no production change is needed for this to pass.
One related inconsistency I will raise separately on #7785: the
`containsKey` branch stores the reported value raw while the insert branch
normalizes, so a non-JSON value flips between `{}` and the original text across
heartbeats. That is #7785 territory, not yours.
If you believe raw pass-through of non-JSON text is the contract you want
instead, say so here — then the right move is to revert the normalize call on
the store path rather than change these assertions, and we should reconcile the
two contracts in one place. Either way master needs to go green before the
2.7.2 cut.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
