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

Add unit tests for host instrumentation #3517

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Petrie
Copy link
Contributor

@Petrie Petrie commented Mar 1, 2023

Resolve #2755

@Petrie Petrie requested a review from a team March 1, 2023 05:04
@Petrie Petrie requested a review from MadVikingGod as a code owner March 1, 2023 05:04
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #3517 (980873e) into main (83a9d91) will increase coverage by 1.9%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3517     +/-   ##
=======================================
+ Coverage   69.9%   71.9%   +1.9%     
=======================================
  Files        147     147             
  Lines       6934    6934             
=======================================
+ Hits        4851    4988    +137     
+ Misses      1959    1807    -152     
- Partials     124     139     +15     
Impacted Files Coverage Δ
instrumentation/host/host.go 76.2% <100.0%> (+76.2%) ⬆️

... and 2 files with indirect coverage changes

@Petrie Petrie force-pushed the host-instrumentation-test branch from 86beff1 to f100d6f Compare March 2, 2023 02:03
@dmathieu
Copy link
Member

dmathieu commented Mar 2, 2023

Adding tests shouldn't need a changelog entry.
Also, what is preventing those tests from residing in the same package as the code?

@Petrie
Copy link
Contributor Author

Petrie commented Mar 2, 2023

@dmathieu

  1. change log will be removed
  2. By adding these tests to a separate test sub-directory, we can avoid unnecessary dependencies on the metric SDK in the host directory.

Signed-off-by: Peter Liu <[email protected]>
@MrAlias MrAlias added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label Mar 2, 2023
.github/dependabot.yml Outdated Show resolved Hide resolved
@Petrie Petrie requested a review from MrAlias March 9, 2023 05:44
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Comment on lines +123 to +130
require.LessOrEqual(t, processBefore.User, processUser)
require.GreaterOrEqual(t, processAfter.User, processUser)
// System times are in range
require.LessOrEqual(t, processBefore.System, processSystem)
require.GreaterOrEqual(t, processAfter.System, processSystem)
// Ranges are not empty
require.NotEqual(t, processAfter.System, processBefore.System)
require.NotEqual(t, processAfter.User, processBefore.User)
Copy link
Member

Choose a reason for hiding this comment

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

In this block we could use assert. here instead of require..

Comment on lines +140 to +147
require.LessOrEqual(t, hostBefore[0].User, systemUser)
require.GreaterOrEqual(t, hostAfter[0].User, systemUser)
// System times are in range
require.LessOrEqual(t, hostBefore[0].System, systemSystem)
require.GreaterOrEqual(t, hostAfter[0].System, systemSystem)
// Ranges are not empty
require.NotEqual(t, hostAfter[0].System, hostBefore[0].System)
require.NotEqual(t, hostAfter[0].User, hostBefore[0].User)
Copy link
Member

Choose a reason for hiding this comment

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

In this block we could use assert. here instead of require..

@pellared
Copy link
Member

@Petrie, are you still willing to work on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add integration testing to the host instrumenation
4 participants