-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Fix AutoUpdatePolicy for channel #3888
Conversation
Fixes openhab#3887 Signed-off-by: Jacob Laursen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. That is really a nasty bug.
....openhab.core.thing/src/main/java/org/openhab/core/thing/binding/builder/ChannelBuilder.java
Outdated
Show resolved
Hide resolved
final ChannelBuilder channelBuilder = ChannelBuilder.create(channelUID, channelType.getItemType()) // | ||
.withType(channelType.getUID()) // | ||
.withDefaultTags(channelType.getTags()) // | ||
.withKind(channelType.getKind()) // | ||
.withLabel(label) // | ||
.withAutoUpdatePolicy(channelType.getAutoUpdatePolicy()); | ||
.withAutoUpdatePolicy(autoUpdatePolicy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test-case for this, too? The test should show that
- no policy is set if neither the channel-definition nor the channel-type-definition set a policy
- if either channel- or channel-type-definition contain a policy, it is used
- if both contain a policy, the channel-definition takes precedence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, you mean a new test class, right? I didn't find any existing coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I need a bit of guidance here. I found some integration tests that seem related:
Lines 347 to 382 in 3624682
@Test | |
public void testCreateChannelGroupBuilder() throws Exception { | |
AtomicReference<ThingHandlerCallback> thc = initializeThingHandlerCallback(); | |
List<ChannelBuilder> channelBuilders = thc.get().createChannelBuilders(CHANNEL_GROUP_UID, | |
CHANNEL_GROUP_TYPE_UID); | |
assertNotNull(channelBuilders); | |
assertEquals(2, channelBuilders.size()); | |
assertNotNull(channelBuilders.get(0)); | |
validateChannel(channelBuilders.get(0).build()); | |
assertNotNull(channelBuilders.get(1)); | |
validateChannelOverridden(channelBuilders.get(1).build()); | |
} | |
private void validateChannel(Channel channel) { | |
assertNotNull(channel); | |
assertEquals("Test Label", channel.getLabel()); | |
assertEquals("Test Description", channel.getDescription()); | |
assertEquals(CoreItemFactory.SWITCH, channel.getAcceptedItemType()); | |
assertEquals(CHANNEL_TYPE_UID, channel.getChannelTypeUID()); | |
assertNotNull(channel.getDefaultTags()); | |
assertEquals(1, channel.getDefaultTags().size()); | |
assertEquals("Test Tag", channel.getDefaultTags().iterator().next()); | |
} | |
private void validateChannelOverridden(Channel channel) { | |
assertNotNull(channel); | |
assertEquals("Test Label Overridden", channel.getLabel()); | |
assertEquals("Test Description Overridden", channel.getDescription()); | |
assertEquals(CoreItemFactory.SWITCH, channel.getAcceptedItemType()); | |
assertEquals(CHANNEL_TYPE_UID, channel.getChannelTypeUID()); | |
assertNotNull(channel.getDefaultTags()); | |
assertEquals(1, channel.getDefaultTags().size()); | |
assertEquals("Test Tag", channel.getDefaultTags().iterator().next()); | |
} |
Did you mean to add new tests here? In that case, how can I build and run those tests?
Signed-off-by: Jacob Laursen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are good for now. We should off proper testing for the whole class in a later PR, that is out of scope here.
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
Fixes #3887