Skip to content

CSHARP-3427: Fix test. #541

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

Merged
merged 2 commits into from
Jun 1, 2021
Merged

Conversation

DmitryLukyanov
Copy link
Contributor

@DmitryLukyanov DmitryLukyanov commented Jun 1, 2021

Copy link
Contributor

@MikalaiMazurenka MikalaiMazurenka left a comment

Choose a reason for hiding this comment

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

I'd recommend to change the name of the commit to be more specific.
My take is "Relax assertion of native library loader test"

@DmitryLukyanov
Copy link
Contributor Author

DmitryLukyanov commented Jun 1, 2021

I'd recommend to change the name of the commit to be more specific.
My take is "Relax assertion of native library loader test"

done

Copy link
Contributor

@MikalaiMazurenka MikalaiMazurenka left a comment

Choose a reason for hiding this comment

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

I think these two cases are better to be split

Comment on lines 32 to 36

// the default assembly-based logic. Ideally expectedRootTestFolder should be mongo-csharp-driver,
// but since it's not mocked logic, it limits us where we can run our tests from. Avoid it by
// making a test assertation less strict
[InlineData(null, "")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using the suggested refactoring: MikalaiMazurenka@bd94d56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the validation and asserting steps here are the same, the only difference is input data, the ideal solution here is ClassData but it looks like overkill for this case, a simple if solves everything we need to. So I would leave this as is

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, then could you remove the extra blank line and consider changing the 3 line comment to

Suggested change
// the default assembly-based logic. Ideally expectedRootTestFolder should be mongo-csharp-driver,
// but since it's not mocked logic, it limits us where we can run our tests from. Avoid it by
// making a test assertation less strict
[InlineData(null, "")]
[InlineData(null, "")] // This case validates only the ending of the path with GetBaseAssemblyUri being used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to explain why this change is necessary since it's kind of not obvious exception, you comment doesn't provide this info, however I don't mind about changing messages, it will be good to hear @JamesKovacs opinion here also

@rstam rstam removed their request for review June 1, 2021 21:23
Copy link
Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@MikalaiMazurenka MikalaiMazurenka left a comment

Choose a reason for hiding this comment

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

LGTM

@DmitryLukyanov DmitryLukyanov merged commit c315f06 into mongodb:master Jun 1, 2021
rstam pushed a commit that referenced this pull request Jun 1, 2021
CSHARP-3427: Relax assertion of native library loader test.
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