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

Fix Windows build of Jaeger tests #1577

Merged
merged 3 commits into from
Feb 24, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Feb 23, 2021

The Jaeger tests use the low-level syscall package. The Windows specific function called in that package has a different function signature than the unix version. Add a windows specific file using the build flags to isolate this OS specific functionality.

I'm able to verify this fixes the compilation of the testing package and the test failure on a Windows system:

go test -timeout 30s -short ./exporters/trace/jaeger/...
ok  	go.opentelemetry.io/otel/exporters/trace/jaeger	2.906s
?   	go.opentelemetry.io/otel/exporters/trace/jaeger/internal/gen-go/jaeger	[no test files]
?   	go.opentelemetry.io/otel/exporters/trace/jaeger/internal/gen-go/jaeger/collector-remote	[no test files]

Resolves #1576

The Jaeger tests use the low-level syscall package. The Windows specific
function called in that package has a different function signature than
the unix version. Add a windows specific file using the build flags to
isolate this OS specific functionality.
@MrAlias MrAlias added the bug Something isn't working label Feb 23, 2021
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #1577 (3b56a12) into main (4a163be) will decrease coverage by 0.0%.
The diff coverage is 44.4%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1577     +/-   ##
=======================================
- Coverage   78.0%   78.0%   -0.1%     
=======================================
  Files        127     128      +1     
  Lines       6597    6606      +9     
=======================================
+ Hits        5151    5155      +4     
- Misses      1201    1204      +3     
- Partials     245     247      +2     
Impacted Files Coverage Δ
exporters/trace/jaeger/assertsocketbuffersize.go 44.4% <44.4%> (ø)

@MrAlias
Copy link
Contributor Author

MrAlias commented Feb 24, 2021

Looking further into the error message from the actual test run, it looks like this will always fail on Windows. The network file descriptor is not available from the connection on Windows. A dup method still needs to be implemented for the netFD on Windows.

https://github.com/golang/go/blob/6cc8aa7ece96aca282db19f08aa5c98ed13695d9/src/net/fd_windows.go#L175-L178

I'm going to updated this to return just return true and add a comment.

@MrAlias MrAlias requested a review from Aneurysm9 February 24, 2021 02:47
@MrAlias MrAlias merged commit c4cf1af into open-telemetry:main Feb 24, 2021
@MrAlias MrAlias deleted the jaeger-win-build branch February 24, 2021 15:01
@Aneurysm9 Aneurysm9 mentioned this pull request Mar 3, 2021
ldelossa pushed a commit to ldelossa/opentelemetry-go that referenced this pull request Mar 5, 2021
* Fix Windows build of Jaeger tests

The Jaeger tests use the low-level syscall package. The Windows specific
function called in that package has a different function signature than
the unix version. Add a windows specific file using the build flags to
isolate this OS specific functionality.

* Add changes to changelog

* Blind succeed to account for unimplemented functionality on Windows
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

exporters/trace/jaeger test compilation error
4 participants