Skip to content

test: add SignalBindingFeature test#23744

Open
tltv wants to merge 3 commits intomainfrom
test/add-ElementBindTextTest
Open

test: add SignalBindingFeature test#23744
tltv wants to merge 3 commits intomainfrom
test/add-ElementBindTextTest

Conversation

@tltv
Copy link
Member

@tltv tltv commented Mar 4, 2026

Test that SignalBindingFeature is known feature with Text component and that getFeatureIfInitialized won't throw IllegalStateException even in case when SignalBindingFeature is not used.

Test that SignalBindingFeature is known feature with Text component and that getFeatureIfInitialized won't throw IllegalStateException even in case when SignalBindingFeature is not used.
@github-actions github-actions bot added the +0.0.1 label Mar 4, 2026
@github-actions
Copy link

github-actions bot commented Mar 4, 2026

Test Results

 1 381 files  ±0   1 381 suites  ±0   1h 27m 30s ⏱️ +34s
 9 846 tests +1   9 775 ✅ +1  71 💤 ±0  0 ❌ ±0 
10 319 runs  +1  10 239 ✅ +1  80 💤 ±0  0 ❌ ±0 

Results for commit d89c947. ± Comparison against base commit efd361d.

♻️ This comment has been updated with latest results.

@tltv tltv added the signals label Mar 4, 2026

@Test
public void missingSignalBindingFeatureDoNotThrow() {
// getFeatureIfInitialized may throw IllegalStateException if the
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it tests the code the way it is implemented. What is unclear is why the caller should not use hasFeature or similar before calling getFeatureIfInitialized, the way the javadoc describes?

Copy link
Member Author

Choose a reason for hiding this comment

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

More verbose and easy to miss it compared to the current way. There were methods earlier that could have used hasFeature instead of catching the exception, but still it's more code compared to having SignalBindingFeature known by default.
Actually now I'm thinking that we could remove the TextBindingFeature and replace it with SignalBindingFeature. The issue revealed this and now I don't see anymore why would we need TextBindingFeature at all.

@Artur-
Copy link
Member

Artur- commented Mar 4, 2026

The test mentioned in the other PR looks much more like what should be tested

button = new Button("foo");
button.bindText(new ValueSignal<>(""));

@tltv
Copy link
Member Author

tltv commented Mar 4, 2026

The test mentioned in the other PR looks much more like what should be tested

Yes but it's in the flow-components. But Button uses Text which is in flow.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 6, 2026

@mshabarov mshabarov self-requested a review March 9, 2026 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants