-
Notifications
You must be signed in to change notification settings - Fork 116
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 URL endpoint for injected NessieApi
from OldNessieServer
#7958
Changes from 4 commits
a6fe710
eee45f1
ed94a2c
ba05a2a
4f67113
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.Optional; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
import org.assertj.core.api.SoftAssertions; | ||
import org.assertj.core.api.junit.jupiter.InjectSoftAssertions; | ||
|
@@ -33,11 +34,13 @@ | |
import org.junit.jupiter.api.extension.ExtendWith; | ||
import org.junit.platform.testkit.engine.EngineExecutionResults; | ||
import org.junit.platform.testkit.engine.EngineTestKit; | ||
import org.junit.platform.testkit.engine.Events; | ||
import org.projectnessie.client.api.NessieApiV1; | ||
import org.projectnessie.junit.engine.MultiEnvTestEngine; | ||
import org.projectnessie.junit.engine.MultiEnvTestFilter; | ||
import org.projectnessie.model.Branch; | ||
import org.projectnessie.model.NessieConfiguration; | ||
import org.projectnessie.model.Reference; | ||
import org.projectnessie.tools.compatibility.api.NessieAPI; | ||
import org.projectnessie.tools.compatibility.api.NessieBaseUri; | ||
import org.projectnessie.tools.compatibility.api.NessieVersion; | ||
|
@@ -48,6 +51,20 @@ | |
class TestNessieCompatibilityExtensions { | ||
@InjectSoftAssertions protected SoftAssertions soft; | ||
|
||
private void assertNoFailedTestEvents(EngineExecutionResults result) { | ||
soft.assertThat(result.testEvents().count()) | ||
.withFailMessage("Failed to run any tests") | ||
.isGreaterThan(0); | ||
|
||
Events failedEvents = result.testEvents().failed(); | ||
soft.assertThat(failedEvents.count()) | ||
.withFailMessage( | ||
() -> | ||
"The following test events have failed:\n:" | ||
+ failedEvents.stream().map(e -> e.toString()).collect(Collectors.joining())) | ||
.isEqualTo(0); | ||
} | ||
|
||
@Test | ||
void noVersions() { | ||
soft.assertThatThrownBy( | ||
|
@@ -100,11 +117,14 @@ void tooManyExtensions() { | |
|
||
@Test | ||
void olderClients() { | ||
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID) | ||
.configurationParameter( | ||
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current") | ||
.selectors(selectClass(OldClientsSample.class)) | ||
.execute(); | ||
XN137 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
EngineExecutionResults results = | ||
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID) | ||
.configurationParameter( | ||
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current") | ||
.selectors(selectClass(OldClientsSample.class)) | ||
.execute(); | ||
assertNoFailedTestEvents(results); | ||
|
||
soft.assertThat(OldClientsSample.allVersions) | ||
.containsExactly(NEW_STORAGE_MODEL_WITH_COMPAT_TESTING, Version.CURRENT); | ||
soft.assertThat(OldClientsSample.minVersionHigh).containsExactly(Version.CURRENT); | ||
|
@@ -115,11 +135,14 @@ void olderClients() { | |
|
||
@Test | ||
void olderServers() { | ||
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID) | ||
.configurationParameter( | ||
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current") | ||
.selectors(selectClass(OldServersSample.class)) | ||
.execute(); | ||
EngineExecutionResults results = | ||
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID) | ||
.configurationParameter( | ||
"nessie.versions", NEW_STORAGE_MODEL_WITH_COMPAT_TESTING.toString() + ",current") | ||
.selectors(selectClass(OldServersSample.class)) | ||
.execute(); | ||
assertNoFailedTestEvents(results); | ||
|
||
soft.assertThat(OldServersSample.allVersions) | ||
.containsExactly(NEW_STORAGE_MODEL_WITH_COMPAT_TESTING, Version.CURRENT); | ||
soft.assertThat(OldServersSample.minVersionHigh).containsExactly(Version.CURRENT); | ||
|
@@ -131,6 +154,23 @@ void olderServers() { | |
soft.assertThat(OldServersSample.uris).allMatch(uri -> uri.getPath().equals("/")); | ||
} | ||
|
||
@Test | ||
void injectedNessieApiUrlChanged() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test fails like this:
|
||
final Version versionBeforeApiUrlChange = Version.parseVersion("0.74.0"); | ||
EngineExecutionResults result = | ||
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID) | ||
.configurationParameter( | ||
"nessie.versions", | ||
versionBeforeApiUrlChange + "," + Version.NESSIE_URL_API_SUFFIX + ",current") | ||
.selectors(selectClass(ApiEndpointServerSample.class)) | ||
.execute(); | ||
|
||
soft.assertThat(ApiEndpointServerSample.allVersions) | ||
.containsExactly(versionBeforeApiUrlChange, Version.NESSIE_URL_API_SUFFIX, Version.CURRENT); | ||
|
||
assertNoFailedTestEvents(result); | ||
} | ||
|
||
@Test | ||
void nestedTests() { | ||
EngineTestKit.engine(MultiEnvTestEngine.ENGINE_ID) | ||
|
@@ -258,6 +298,32 @@ void never() { | |
} | ||
} | ||
|
||
@ExtendWith({OlderNessieServersExtension.class, SoftAssertionsExtension.class}) | ||
static class ApiEndpointServerSample { | ||
@InjectSoftAssertions protected SoftAssertions soft; | ||
|
||
@NessieAPI NessieApiV1 api; | ||
@NessieAPI static NessieApiV1 apiStatic; | ||
@NessieBaseUri URI uri; | ||
@NessieBaseUri static URI uriStatic; | ||
@NessieVersion Version version; | ||
@NessieVersion static Version versionStatic; | ||
|
||
static final List<Version> allVersions = new ArrayList<>(); | ||
|
||
@Test | ||
void testSome() throws Exception { | ||
soft.assertThat(api).isNotNull().isSameAs(apiStatic); | ||
soft.assertThat(uri).isNotNull().isEqualTo(uriStatic); | ||
soft.assertThat(version).isNotNull().isEqualTo(versionStatic); | ||
allVersions.add(version); | ||
|
||
// use api | ||
List<Reference> references = api.getAllReferences().get().getReferences(); | ||
soft.assertThat(references).isNotEmpty(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note that without |
||
} | ||
} | ||
|
||
@ExtendWith(OlderNessieServersExtension.class) | ||
static class OuterSample { | ||
static final List<Version> outerVersions = new ArrayList<>(); | ||
|
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.
#7854 added
api/
here but nothing version-aware inOldNessieServer
any ideas how to stay "ahead" of such problems in the future?
i.e. do we need server/client compatibility tests that use a
Version.LAST_RELEASE
value? then at least after a new release we might get notified.or a test that uses
Version.CURRENT
withOldNessieServer
somehow?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.
OldNessieServer
relies on published jars, so it is currently at least one release behind (it cannot testmain
due to lack of artifacts). One option may be to dopublishToMavenLocal
and use those artifacts in CI.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.
Add the version to
nessie-compatibility.properties
?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.
If we go with
publishToMavenLocal
I believe some extra changes will be required to support "snapshot" versions.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.
doesnt this require awareness of the developer that made the change that compatibility was broken?
CI tests should find this for the developer "automatically"
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.
What I'm proposing is to run
publishToMavenLocal
in CI and use that version for tests automatically, indeed... but let's do that in another PR (I believe it will have some ripple effect in the code that handles versions).