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 unit tests on Windows #983

Merged

Conversation

james-bebbington
Copy link
Member

@james-bebbington james-bebbington commented May 18, 2020

Description:
Fixes several unit tests that were failling on Windows:

  1. configgrpc, configgrpc_test & opencensusexporter.factory: On Windows, calling x509.SystemCertPool() directly returns an error (see the discussion here crypto/x509: make SystemCertPool work on Windows? golang/go#16736), but as per the documentation, leaving TLSConfig.RootCAs as nil will still correctly validate certificates against the system cert pool. Fixing the failing tests on Windows required some minor code changes so that we never load the system cert pool, and just rely on RootCAs being nil instead. This made an existing test that checked if the call to x509.SystemCertPool() failed redundant.

  2. configgrpc_test: Generalised a couple of checks against expected errors so they will pass on Windows (due to slightly different error responses).

  3. opencensus_test & otlp_test: A tmp file was opened and then deleted before being closed, which raises a file-in-use error on Windows ==> Close the file before deleting it.

Link to tracking Issue:
#854

See also:
#976

go.mod Outdated
@@ -56,7 +56,7 @@ require (
go.opencensus.io v0.22.3
go.uber.org/zap v1.10.0
golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e // indirect
golang.org/x/sys v0.0.0-20200408040146-ea54a3c99b9b // indirect
golang.org/x/sys v0.0.0-20200408040146-ea54a3c99b9b
Copy link
Member Author

Choose a reason for hiding this comment

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

Sneaking this change in. A direct reference to x/sys was actually introduced in #953

Copy link
Member

Choose a reason for hiding this comment

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

Would suggest to always split in separate PRs :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright 👍 Rolled this back in this branch

@james-bebbington james-bebbington changed the title Minor changes so all tests can run succesfully on Windows Fix unit tests on Windows May 18, 2020
@codecov-io
Copy link

codecov-io commented May 18, 2020

Codecov Report

Merging #983 into master will decrease coverage by 0.00%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
- Coverage   85.62%   85.62%   -0.01%     
==========================================
  Files         184      184              
  Lines       13087    13080       -7     
==========================================
- Hits        11206    11200       -6     
+ Misses       1434     1432       -2     
- Partials      447      448       +1     
Impacted Files Coverage Δ
exporter/opencensusexporter/factory.go 93.54% <66.66%> (+4.31%) ⬆️
config/configgrpc/configgrpc.go 100.00% <100.00%> (ø)
receiver/opencensusreceiver/octrace/opencensus.go 90.00% <0.00%> (-3.34%) ⬇️
translator/internaldata/resource_to_oc.go 76.47% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a637b41...edb7e82. Read the comment docs.

config/configgrpc/configgrpc_test.go Show resolved Hide resolved
go.mod Outdated
@@ -56,7 +56,7 @@ require (
go.opencensus.io v0.22.3
go.uber.org/zap v1.10.0
golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e // indirect
golang.org/x/sys v0.0.0-20200408040146-ea54a3c99b9b // indirect
golang.org/x/sys v0.0.0-20200408040146-ea54a3c99b9b
Copy link
Member

Choose a reason for hiding this comment

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

Would suggest to always split in separate PRs :)

receiver/otlpreceiver/otlp_test.go Outdated Show resolved Hide resolved
@james-bebbington james-bebbington force-pushed the fix-unit-tests-for-windows branch from edb7e82 to b0df43d Compare May 19, 2020 01:28
@codecov-commenter
Copy link

Codecov Report

Merging #983 into master will increase coverage by 0.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
+ Coverage   85.62%   85.64%   +0.01%     
==========================================
  Files         184      184              
  Lines       13087    13080       -7     
==========================================
- Hits        11206    11202       -4     
+ Misses       1434     1431       -3     
  Partials      447      447              
Impacted Files Coverage Δ
exporter/opencensusexporter/factory.go 93.54% <66.66%> (+4.31%) ⬆️
config/configgrpc/configgrpc.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 76.47% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a637b41...b0df43d. Read the comment docs.

@bogdandrutu bogdandrutu merged commit e641384 into open-telemetry:master May 19, 2020
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Remove otel/sdk dependency from grpctrace

Use otel/trace/testtrace instead and cleanup testing code.

* Update httptrace to not depend on the SDK

Update testing to use api/trace/testtrace instead.

* Add changes to Changelog

* Restore check for `http.local` attr on `http.getconn`
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
…y#983)

Bumps [boto3](https://github.com/boto/boto3) from 1.20.12 to 1.20.13.
- [Release notes](https://github.com/boto/boto3/releases)
- [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst)
- [Commits](boto/boto3@1.20.12...1.20.13)

---
updated-dependencies:
- dependency-name: boto3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024
* update to demo 1.7

Signed-off-by: Pierre Tessier <pierre@pierretessier.com>

* update to demo 1.7

Signed-off-by: Pierre Tessier <pierre@pierretessier.com>

---------

Signed-off-by: Pierre Tessier <pierre@pierretessier.com>
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.

4 participants