[awattar] Feature/push code coverage

[tl-photography]

PR does not change behaviour.

Only pushes code coverage and code maintainability.

[jlaur]

@tl-photography - thanks. Did you intend to include the two commits from #17729? In that case, we need to add label "awaiting other PR" to mention this dependency.

[tl-photography]

Ah ok, I marked it as draft only.

Can I set this label too, otherwise please do so.

[Wolfgang1966]

LGTM. Thanks for your work and your patience!

[lolodomo]

I just merged #17729

[tl-photography]

@lolodomo

Thanks! PR should be good. 👍

Edit: Should I rebase?

[jlaur]

Edit: Should I rebase?

Yes, please.

[tl-photography]

Edit: Should I rebase?

Yes, please.

done

[jlaur]

Here and other places below: This can be simplified, there is no reason to go through the trouble of getting a ZonedDateTime just to discard to time-zone again when converting back to Instant:

Suggested change

	state = OnOffType.from(result.isActive(getNow(zoneId).toInstant()));
	state = OnOffType.from(result.isActive(Instant.now()));

[tl-photography]

The main reason for this change is to access the instant in the utests for stubbing.
I did not find a way with the available framework to stub the const Instant.now() method.

[tl-photography]

hwo about this change in 622cfc0

would this work for you?

[jlaur]

You might also be able to provide a Clock object for mocking this. See for example:

openhab-addons/bundles/org.openhab.binding.energidataservice/src/main/java/org/openhab/binding/energidataservice/internal/PriceListParser.java

Lines 41 to 59 in 98d2579

	private final Clock clock;
	public PriceListParser() {
	this(Clock.system(EnergiDataServiceBindingConstants.DATAHUB_TIMEZONE));
	}
	public PriceListParser(Clock clock) {
	this.clock = clock;
	}
	public Map<Instant, BigDecimal> toHourly(Collection<DatahubPricelistRecord> records) {
	Instant firstHourStart = Instant.now(clock)
	.minus(ElectricityPriceSubscriptionCache.NUMBER_OF_HISTORIC_HOURS, ChronoUnit.HOURS)
	.truncatedTo(ChronoUnit.HOURS);
	Instant lastHourStart = Instant.now(clock).truncatedTo(ChronoUnit.HOURS).plus(2, ChronoUnit.DAYS)
	.truncatedTo(ChronoUnit.DAYS);
	return toHourly(records, firstHourStart, lastHourStart);
	}

and:

openhab-addons/bundles/org.openhab.binding.energidataservice/src/test/java/org/openhab/binding/energidataservice/internal/PriceListParserTest.java

Lines 67 to 79 in 98d2579

	@Test
	void toHourlyNoChanges() throws IOException {
	PriceListParser priceListParser = new PriceListParser(
	Clock.fixed(Instant.parse("2023-01-23T12:00:00Z"), EnergiDataServiceBindingConstants.DATAHUB_TIMEZONE));
	DatahubPricelistRecords records = getObjectFromJson("DatahubPricelistN1.json", DatahubPricelistRecords.class);
	Map<Instant, BigDecimal> tariffMap = priceListParser.toHourly(Arrays.stream(records.records()).toList());
	assertThat(tariffMap.size(), is(60));
	assertThat(tariffMap.get(Instant.parse("2023-01-23T15:00:00Z")), is(equalTo(new BigDecimal("0.432225"))));
	assertThat(tariffMap.get(Instant.parse("2023-01-23T16:00:00Z")), is(equalTo(new BigDecimal("1.05619"))));
	assertThat(tariffMap.get(Instant.parse("2023-01-24T15:00:00Z")), is(equalTo(new BigDecimal("0.432225"))));
	assertThat(tariffMap.get(Instant.parse("2023-01-24T16:00:00Z")), is(equalTo(new BigDecimal("1.05619"))));
	}

[tl-photography]

I just had a look and it seems the example you send me implements a Constructor where the clock object is initialized exactly for the reason of testing.
In the production code, only the constructor without the parameter is used.

I can change the code, but i think there is not much difference between the two approaches.

[tl-photography]

i have worked on it and changed most of the code to a clock object to have the same look and feel.

[tl-photography]

i had conflicts and resolved them via rebase

[tl-photography]

I also added now tests for the refresh logic, since this was easy to add after the clock rework.

[tl-photography]

Coverage is now at 57% instr respectively 51% branch coverage.