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

Move Scraper test functionality to seperate package #5755

Merged
merged 23 commits into from
Oct 29, 2021

Conversation

Mrod1598
Copy link
Contributor

@Mrod1598 Mrod1598 commented Oct 14, 2021

Description:

Moved common scraper test code to separate package

Link to tracking Issue:
Resolves #5741

Testing:
Added testing to confirm testing ability of CompareMetrics Func.

internal/scrapertest/testdata/scrapers/expected.json Outdated Show resolved Hide resolved
internal/scrapertest/validation.go Outdated Show resolved Hide resolved
internal/scrapertest/validation.go Outdated Show resolved Hide resolved
internal/scrapertest/validation.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest.go Outdated Show resolved Hide resolved
@Mrod1598
Copy link
Contributor Author

Also, separated out the compare function into multiple different funcs for easier readability. Also, it could be helpful to compare at different levels.

@Mrod1598 Mrod1598 marked this pull request as ready for review October 21, 2021 22:07
@Mrod1598 Mrod1598 requested a review from a team October 21, 2021 22:07
@djaglowski
Copy link
Member

@Mrod1598 I like where this is going. A few more things to address, but overall this is what I wanted to see.

@dmitryax, you might be interested to evaluate this. I expect this would be a very useful testing pattern for scrapers. Initially the package is proposed as internal, but could move elsewhere if/when appropriate.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Overall LGTM

internal/scrapertest/scrapertest.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest_test.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest_test.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest_test.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest_test.go Outdated Show resolved Hide resolved
internal/scrapertest/scrapertest_test.go Outdated Show resolved Hide resolved
@djaglowski djaglowski added the ready to merge Code review completed; ready to merge by maintainers label Oct 26, 2021
@djaglowski
Copy link
Member

@Mrod1598 please resolve the conflicts

@bogdandrutu bogdandrutu removed the ready to merge Code review completed; ready to merge by maintainers label Oct 27, 2021
@bogdandrutu
Copy link
Member

@djaglowski please readd the label when PR is rebased

@Mrod1598
Copy link
Contributor Author

Mrod1598 commented Oct 27, 2021

@Mrod1598 please resolve the conflicts

resolved

@djaglowski djaglowski added the ready to merge Code review completed; ready to merge by maintainers label Oct 28, 2021
@djaglowski
Copy link
Member

@Mrod1598 Please resolve conflicts

@djaglowski djaglowski removed the ready to merge Code review completed; ready to merge by maintainers label Oct 29, 2021
@djaglowski djaglowski added the ready to merge Code review completed; ready to merge by maintainers label Oct 29, 2021
@bogdandrutu bogdandrutu merged commit d2c9ddf into open-telemetry:main Oct 29, 2021
@Mrod1598 Mrod1598 deleted the update-scraper-tests branch October 29, 2021 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract scraper test code into common package
5 participants