Skip to content
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.

Sign up for GitHub

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

chore: update yggdrasil to handle latest spec #261

Merged
merged 4 commits into from
Jan 16, 2025
Merged

chore: update yggdrasil to handle latest spec #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d7e5ab
chore: pull engine version from yggdrasil
sighphyre Jan 16, 2025
5de3208
fix: error handling
sighphyre Jan 16, 2025
5641bf9
wip: add sleep because it looks like the failing test is a concurrenc…
sighphyre Jan 16, 2025
91aa034
fix: make client spec tests predictably ordered (#262)
sighphyre Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
<version.junit5>5.10.3</version.junit5>
<version.okhttp>4.12.0</version.okhttp>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<version.unleash.specification>5.1.7</version.unleash.specification>
<version.unleash.specification>5.1.9</version.unleash.specification>
<arguments />
<version.jackson>2.17.2</version.jackson>
<version.logback>1.3.14</version.logback>
Expand Down Expand Up @@ -59,7 +59,7 @@
<dependency>
<groupId>io.getunleash</groupId>
<artifactId>yggdrasil-engine</artifactId>
<version>0.1.0-alpha.12</version>
<version>0.1.0-alpha.14</version>
<classifier>${os.detected.classifier}</classifier>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/getunleash/DefaultUnleash.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public DefaultUnleash(
try {
this.unleashEngine.takeState(
JsonFeatureParser.toJsonString(featureCollection));
} catch (YggdrasilInvalidInputException e) {
} catch (YggdrasilInvalidInputException | YggdrasilError e) {
LOGGER.error("Unable to update features", e);
}
});
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/io/getunleash/metric/ClientMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.getunleash.metric;

import io.getunleash.engine.MetricsBucket;
import io.getunleash.engine.UnleashEngine;
import io.getunleash.event.UnleashEvent;
import io.getunleash.event.UnleashSubscriber;
import io.getunleash.lang.Nullable;
Expand All @@ -25,7 +26,7 @@ public class ClientMetrics implements UnleashEvent {
this.specVersion = config.getClientSpecificationVersion();
this.platformName = System.getProperty("java.vm.name");
this.platformVersion = System.getProperty("java.version");
this.yggdrasilVersion = null;
this.yggdrasilVersion = UnleashEngine.getCoreVersion();
}

public String getAppName() {
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/io/getunleash/metric/ClientRegistration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.getunleash.metric;

import io.getunleash.engine.UnleashEngine;
import io.getunleash.event.UnleashEvent;
import io.getunleash.event.UnleashSubscriber;
import io.getunleash.lang.Nullable;
Expand Down Expand Up @@ -31,7 +32,7 @@ public class ClientRegistration implements UnleashEvent {
this.specVersion = config.getClientSpecificationVersion();
this.platformName = System.getProperty("java.vm.name");
this.platformVersion = System.getProperty("java.version");
this.yggdrasilVersion = null;
this.yggdrasilVersion = UnleashEngine.getCoreVersion();
}

public String getAppName() {
Expand Down
1 change: 1 addition & 0 deletions src/test/java/io/getunleash/DefaultUnleashTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ public void asynchronous_fetch_on_initialisation_fails_silently_and_retries()
.build();

Unleash unleash = new DefaultUnleash(config);
Thread.sleep(1);
Copy link
Member Author

Choose a reason for hiding this comment

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

cries in Windows So this is the failing test that sent me down the rabbit hole of the client spec tests (which aren't yet fixed). It looks like there's a very subtle difference in the thread scheduler between Windows and Linux. Linux is more precise, which means that the fetch has always completed before the verify runs on the next line. On Windows, this never completes without this sleep

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can't wait on DefaultUnleash constructor, but tbh, I don't like it does fetchFeatures as part of constructing the object... I think fetch features should be part of some initialize() method and then maybe you'd be able to wait on it... but sleeping is fine, people need to sleep :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I'd like it to change too and what you propose is what I'd like to do too, Constructors should construct, not spawn threads.

Small steps though, this is legacy behaviour that's being surfaced because we're doing Windows tests now. Can fix next series of PRs

verify(fetcher, times(1)).fetchFeatures();
Thread.sleep(1200);
verify(fetcher, times(2)).fetchFeatures();
Expand Down
61 changes: 3 additions & 58 deletions src/test/java/io/getunleash/integration/ClientSpecificationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,8 @@
package io.getunleash.integration;

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.equalTo;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.post;
import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
import com.google.gson.Gson;
import com.google.gson.reflect.TypeToken;
import io.getunleash.DefaultUnleash;
Expand All @@ -21,8 +13,6 @@
import io.getunleash.strategy.constraints.DateParser;
import io.getunleash.util.UnleashConfig;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
Expand All @@ -35,17 +25,9 @@
import java.util.stream.Stream;
import org.junit.jupiter.api.DynamicTest;
import org.junit.jupiter.api.TestFactory;
import org.junit.jupiter.api.extension.RegisterExtension;

public class ClientSpecificationTest {

@RegisterExtension
static WireMockExtension serverMock =
WireMockExtension.newInstance()
.configureStaticDsl(true)
.options(wireMockConfig().dynamicPort().dynamicHttpsPort())
.build();

@TestFactory
public Stream<DynamicTest> clientSpecification() throws IOException, URISyntaxException {
Reader content = getFileReader("/client-specification/specifications/index.json");
Expand Down Expand Up @@ -122,20 +104,14 @@ private List<DynamicTest> createVariantTests(String fileName)
}

private Unleash setupUnleash(TestDefinition testDefinition) throws URISyntaxException {
mockUnleashAPI(testDefinition);

// Required because the client is available before it may have had the chance to
// talk with
// the API
String backupFile = writeUnleashBackup(testDefinition);

// Set-up a unleash instance, using mocked API and backup-file
UnleashConfig config =
UnleashConfig.builder()
.appName(testDefinition.getName())
.unleashAPI(new URI("http://localhost:" + serverMock.getPort() + "/api/"))
.synchronousFetchOnInitialisation(true)
.backupFile(backupFile)
.disableMetrics()
.disablePolling()
.unleashAPI(new URI("http://notusedbutrequired:9999/api/"))
.build();

DefaultUnleash defaultUnleash = new DefaultUnleash(config);
Expand All @@ -144,18 +120,6 @@ private Unleash setupUnleash(TestDefinition testDefinition) throws URISyntaxExce
return defaultUnleash;
}

private void mockUnleashAPI(TestDefinition definition) {
stubFor(
get(urlEqualTo("/api/client/features"))
.withHeader("Accept", equalTo("application/json"))
.willReturn(
aResponse()
.withStatus(200)
.withHeader("Content-Type", "application/json")
.withBody(definition.getState().toString())));
stubFor(post(urlEqualTo("/api/client/register")).willReturn(aResponse().withStatus(200)));
}

private TestDefinition getTestDefinition(String fileName) throws IOException {
Reader content = getFileReader("/client-specification/specifications/" + fileName);
return new Gson().fromJson(content, TestDefinition.class);
Expand Down Expand Up @@ -190,23 +154,4 @@ private Reader getFileReader(String filename) throws IOException {
InputStreamReader reader = new InputStreamReader(in);
return new BufferedReader(reader);
}

private String writeUnleashBackup(TestDefinition definition) {
String backupFile =
System.getProperty("java.io.tmpdir")
+ File.separatorChar
+ "unleash-test-"
+ definition.getName()
+ ".json";

// TODO: we can probably drop this after introduction of
// `synchronousFetchOnInitialisation`.
try (FileWriter writer = new FileWriter(backupFile)) {
writer.write(definition.getState().toString());
} catch (IOException e) {
System.out.println("Unable to write toggles to file");
}

return backupFile;
}
}
4 changes: 2 additions & 2 deletions src/test/java/io/getunleash/metric/UnleashMetricServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ public void should_add_new_metrics_data_to_bucket() {
verify(sender, times(1)).sendMetrics(metricsSent.capture());
ClientMetrics metrics = metricsSent.getValue();
assertThat(metrics.getSpecVersion()).isNotEmpty();
assertThat(metrics.getYggdrasilVersion()).isNull();
assertThat(metrics.getYggdrasilVersion()).isNotEmpty();
assertThat(metrics.getPlatformName()).isNotEmpty();
assertThat(metrics.getPlatformVersion()).isNotEmpty();
}
Expand All @@ -467,6 +467,6 @@ public void client_registration_also_includes_new_metrics_metadata() {
assertThat(reg.getPlatformName()).isNotEmpty();
assertThat(reg.getPlatformVersion()).isNotEmpty();
assertThat(reg.getSpecVersion()).isEqualTo(config.getClientSpecificationVersion());
assertThat(reg.getYggdrasilVersion()).isNull();
assertThat(reg.getYggdrasilVersion()).isNotEmpty();
}
}
2 changes: 1 addition & 1 deletion src/test/java/io/getunleash/repository/UnleashEngineStateHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public MetricsBucket captureMetrics() {
public void setState(String raw) {
try {
this.unleashEngine.takeState(raw);
} catch (YggdrasilInvalidInputException e) {
} catch (YggdrasilInvalidInputException | YggdrasilError e) {
throw new RuntimeException(e);
}
}
Expand Down
Loading