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

Skip all NSURL* test cases using GSInetServerStream on Windows #268

Merged
merged 11 commits into from
Sep 27, 2022

Conversation

hmelder
Copy link
Contributor

@hmelder hmelder commented Aug 23, 2022

No description provided.

@hmelder hmelder requested a review from rfm as a code owner August 23, 2022 07:51
@hmelder
Copy link
Contributor Author

hmelder commented Aug 23, 2022

Btw I think we should deprecate GSServerStream. It is not used anywhere in the code base and only exposed as a category in GNUstep additions. The implementation is too complicated for what it does.

@hmelder
Copy link
Contributor Author

hmelder commented Aug 27, 2022

Is it ok to merge @rfm ?

@rfm
Copy link
Contributor

rfm commented Aug 30, 2022

I'm not sure of the rationale here.
While GSServerStream is (so far as i can see) only used in NSSetServices in the base library and in a few testcases, I wonder if it's really not useful / too complicated? I think it would be fine to deprecate and then replace it in those places (and remove it from the additions api) if it's easy.
But, if it's simple to replace, why not do that rather than removing the tests?

@hmelder
Copy link
Contributor Author

hmelder commented Aug 30, 2022

But, if it's simple to replace, why not do that rather than removing the tests?

The unit tests implement multiple test server based on GSServerStream. Rewriting all of them would be a major undertaking. I think we should temporarily disable them.

@triplef
Copy link
Member

triplef commented Sep 11, 2022

#266 has some more info about the issues with GSServerStream on Windows.

For now it would be great if we can mark the tests using it as hopeful on Windows (as this PR does). This will allow us to disable allow-test-failures on the Windows CI targets, so that possible future changes breaking Windows builds will actually fail those builds.

@rfm
Copy link
Contributor

rfm commented Sep 15, 2022

Marking the tests as hopeful seems fine to me.

@hmelder
Copy link
Contributor Author

hmelder commented Sep 15, 2022

Marking the tests as hopeful seems fine to me.

This was my original plan, but I ran into some issues back then. I can look into this and update the PR.

@hmelder
Copy link
Contributor Author

hmelder commented Sep 15, 2022

Forgot the semicolons. Wrote to much golang :)

@hmelder
Copy link
Contributor Author

hmelder commented Sep 15, 2022

Done! We have no test failures in Windows MinGW GCC x86_64 and Windows MSVC x86_64. There is still an unrelated test failure on 32-bit machines. Ready to check @rfm

@triplef
Copy link
Member

triplef commented Sep 15, 2022

Awesome, great job @hmelder! 🎉

I think this should be removed then as well (for both 64-bit Windows targets):

allow-test-failures: true

@hmelder
Copy link
Contributor Author

hmelder commented Sep 22, 2022

@rfm ready to review

@hmelder hmelder merged commit a8c421d into master Sep 27, 2022
@hmelder hmelder deleted the NSURL-win32-hopeful branch September 27, 2022 18:21
@RoyalStewart90
Copy link
Contributor

Hi @hmelder, very sorry to be this late to the party, earlier this year I was poking around the IPv6 functionality and made some changes to my company's branch of Master. In your experience, what was the issue with GSInetServerStream? I had encountered a few issues of formatting IPv6 address with brackets related to GSStreams. The biggest issue I ran into was memory allocation of IP addresses. I'm hoping at some point we can get this issue fixed and re-enable more connection tests.

@hmelder
Copy link
Contributor Author

hmelder commented Oct 31, 2023

@RoyalStewart90 see #266

I think it was an issue with the asynchronous event handling on Windows. I would have to go into the issue as this was about a year ago :)

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

Successfully merging this pull request may close these issues.

4 participants