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

Jump to bottom

Adds tests for ArangoDB 3.12 #499

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Adds tests for ArangoDB 3.12 #499

wants to merge 5 commits into from

Conversation

DiscoPYF
Copy link
Collaborator

fix #498

  • Add a new CI job to run tests against ArangoDB 3.12 in docker.
  • Update older jobs to run against latest minor version by using shorter docker tag, e.g. 3.9 instead of 3.9.1

@DiscoPYF DiscoPYF requested a review from rossmills99 February 17, 2025 15:05
@DiscoPYF
Copy link
Collaborator Author

Some tests are failing because a new error code is returned.

For example in DeleteVertexAsync_ShouldThrow_WhenVertexCollectionIsNotFound, we used to get 1203 ARANGO_DATA_SOURCE_NOT_FOUND (3.11 and lower) but now get 1949 ERROR_GRAPH_COLLECTION_NOT_PART_OF_THE_GRAPH with 3.12.

image

I'll update the tests.
❓ Maybe we drop the error code assertion? It's ArangoDB's job to test the API contract.

@rossmills99
Copy link
Collaborator

Maybe we drop the error code assertion? It's ArangoDB's job to test the API contract.

I think we are also testing deserialization at least, so that has value in my view. Also it is a good way to find out when breaking changes occur since this doesn't get a mention in the release notes as far as I can tell https://docs.arangodb.com/3.12/release-notes/version-3.12/incompatible-changes-in-3-12/

@DiscoPYF
Copy link
Collaborator Author

DiscoPYF commented Feb 18, 2025 •
edited
Loading

As discussed separately with @rossmills99, plan is to:

  • Create a condition in the tests to assert for 1949 if ArangoDB version is superior or equal to 3.12, otherwise 1203. We already have a mechanism in place to check the version in tests. Each test fixture class has access to VersionMajor and VersionMinor.

  • Add the new error code to enum ArangoDBErrors and use it in the tests. I opened a separate PR to update the enum: Create enum generator for ArangoDBErrors and generate change for ArangoDB 3.12 #503

@DiscoPYF
Copy link
Collaborator Author

DiscoPYF commented Mar 10, 2025 •
edited
Loading

Update:

  • Rebased on master
  • Update all error code assertions to use enum ArangoDBErrors
  • Handle breaking change from ArangoDB 3.12 by asserting different error codes depending on the ArangoDB version.

Ready to review.

DiscoPYF
DiscoPYF commented Mar 10, 2025
View reviewed changes
arangodb-net-standard.Test/GraphApi/GraphApiClientTest.cs
@@ -81,7 +83,7 @@ public async Task DeleteGraphAsync_ShouldThrow_WhenNotFound()
});

Assert.Equal(HttpStatusCode.NotFound, exception.ApiError.Code);
Assert.Equal(1924, exception.ApiError.ErrorNum); // GRAPH_NOT_FOUND
Assert.Equal((int)ERROR_GRAPH_NOT_FOUND, exception.ApiError.ErrorNum);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🌱 Perhaps int constants would be better instead of enum values.

public static ArangoDBErrors
{
  public const int ERROR_GRAPH_NOT_FOUND = 1924;
}

It would be a breaking change in theory, but I can't imagine consumers doing anything else than int comparison against ApiErrorResponse.ErrorNum.

@DiscoPYF
Copy link
Collaborator Author

There are still test failures for CollectionApiClientTest, I will look into that this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rossmills99 rossmills99 Awaiting requested review from rossmills99

Assignees
No one assigned
Labels
ArangoDB 3.12
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run tests against ArangoDB 3.12
2 participants
@DiscoPYF @rossmills99