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

Added failing test: ClientPutFile_ZeroByteFile #2

Merged
merged 10 commits into from
Mar 7, 2025
Merged

Conversation

icnocop
Copy link
Contributor

@icnocop icnocop commented Mar 7, 2025

No description provided.

@adstep
Copy link
Owner

adstep commented Mar 7, 2025

The changes look good. Are you able to fix the failing tests?

It's been a while since I worked on the project. I believe they worked using a recording of a real session and played back the underlying http messages.

@icnocop
Copy link
Contributor Author

icnocop commented Mar 7, 2025

I just added another commit which fixes the issue.

Thank you.

@icnocop icnocop marked this pull request as draft March 7, 2025 09:05
@icnocop
Copy link
Contributor Author

icnocop commented Mar 7, 2025

For some reason, the session recorded for the new test ClientPutFile_ZeroByteFile when targeting net7 succeed when played back in tests targeting net6 and net7, but fail when targeting netcoreapp3.1 and net5.0.

@adstep
Copy link
Owner

adstep commented Mar 7, 2025

Does it repro locally for you? We can modify the targets if needed.

@icnocop
Copy link
Contributor Author

icnocop commented Mar 7, 2025

Yes, I can reproduce the issue locally.

Should I remove netcoreapp3.1 and net5.0?
Should I add net8.0?

@adstep
Copy link
Owner

adstep commented Mar 7, 2025

Should I remove netcoreapp3.1 and net5.0?

Sure we can deprecate those if they are causing you trouble.

Should I add net8.0?

If that's useful for you feel free. Or you can add net9.0.

Removed netcoreapp3.1 and net5.0
Added net8.0 and net9.0
@adstep
Copy link
Owner

adstep commented Mar 7, 2025

Looks like the build is passing. If you want to flip this off draft we can get this merged. I'll take a crack at publishing the package this weekend.

@icnocop icnocop marked this pull request as ready for review March 7, 2025 23:54
@icnocop
Copy link
Contributor Author

icnocop commented Mar 7, 2025

Thank you!

@adstep adstep merged commit 17a21e7 into adstep:main Mar 7, 2025
2 checks passed
@adstep
Copy link
Owner

adstep commented Mar 7, 2025

Thanks for your contribution. Happy you find it of use.

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.

2 participants