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.

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

CSHARP-5423: Remove deprecated field from GridFS unified tests #1579

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

papafe
Copy link
Contributor

@papafe papafe commented Dec 19, 2024

No description provided.

@papafe papafe requested a review from BorisDog December 19, 2024 12:07
@papafe papafe requested a review from a team as a code owner December 19, 2024 12:07
@papafe
Copy link
Contributor Author

papafe commented Dec 19, 2024

Not sure if we want to keep them, but for now I've moved the legacy tests inside the "legacy" folder.

Copy link
Contributor

@rstam rstam left a comment

Choose a reason for hiding this comment

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

Please make the commit tree linear before we review.

@papafe
Copy link
Contributor Author

papafe commented Dec 19, 2024

@rstam Rebased on main

@papafe papafe marked this pull request as draft December 19, 2024 18:20
@papafe
Copy link
Contributor Author

papafe commented Dec 19, 2024

Converted it to draft as I need to finish a couple of things

@papafe papafe marked this pull request as ready for review December 20, 2024 11:26
@papafe
Copy link
Contributor Author

papafe commented Dec 20, 2024

@BorisDog regarding the deprecated fields, namely "contentType" and "aliases", I've removed them from the registered members of the serializers and also from a test method input given they were not used.
Apart from that, no need to do more, since all the extra members are just kept into GridFSFileInfo.BackingDocument (a BsonDocument). This test verifies that:

[Theory]
[ParameterAttributeData]
public void ExtraElements_should_be_deserialized_correctly(
[Values(new string[0], new[] { "x" }, new[] { "x", "y" }, new[] { "ExtraElements" })]
string[] names)
{
var document = CreateFilesCollectionDocument();
var extraElements = new BsonDocument();
var value = 1;
foreach (var name in names)
{
extraElements.Add(name, value++);
}
document.Merge(extraElements, overwriteExistingElements: false);
var subject = DeserializeFilesCollectionDocument(document);
foreach (var element in extraElements)
{
subject.BackingDocument[element.Name].Should().Be(element.Value);
}
}

Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

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

Looks good overall.
There was some confusion with the legacy tests, as I thought the spec contains both legacy and unified folders.
We need to align with the spec tests structure, and have just unified tests (and remove legacy runners).
So we need to do CSHARP-3587 as prerequisite. We could also address CSHARP-3587 as part of this ticket.

@@ -0,0 +1,97 @@
/* Copyright 2020-present MongoDB Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright 2010

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected. I noticed that there are other files with various years, mostly in the test project.

@papafe papafe requested a review from BorisDog December 23, 2024 10:35
@papafe
Copy link
Contributor Author

papafe commented Dec 23, 2024

@BorisDog, I've removed the legacy tests and their runners, so I suppose CSHARP-3587 could also be closed when this is merged

Copy link
Contributor

@BorisDog BorisDog left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants