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.

Sign up for GitHub

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

Jump to bottom

Add test for tests. #4063

Closed
wants to merge 2 commits into from
Closed

Add test for tests. #4063

wants to merge 2 commits into from

Conversation

scr-oath
Copy link
Contributor

@github-actions GitHub Actions
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, d7c782f

adapterstest

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/adapterstest/adapter_test_util.go:23:	BidOnTags			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/adapter_test_util.go:33:	SampleBid			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/adapter_test_util.go:46:	VerifyStringValue		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/adapter_test_util.go:53:	VerifyIntValue			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/adapter_test_util.go:60:	VerifyBoolValue			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/adapter_test_util.go:67:	VerifyBannerSize		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:70:		RunJSONBidderTest		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:83:		RunSingleJSONBidderTest		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:105:		loadFile			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:127:		runSpec				0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:135:		getTestExtraRequestInfo		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:185:		expectsErrors			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:194:		ToRequestData			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:215:		ToResponseData			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:244:		assertMakeRequestsOutput	0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:262:		assertErrorList			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:287:		assertMakeBidsOutput		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:307:		diffHttpRequests		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:342:		diffBids			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:357:		diffOrtbBids			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:370:		diffBidVideo			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:381:		diffJson			0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:419:		testMakeRequestsImpl		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:440:		getDataRaceTestCopies		0.0%
github.com/prebid/prebid-server/v3/adapters/adapterstest/test_json.go:461:		testMakeBidsImpl		0.0%
total:											(statements)			0.0%

SyntaxNode
SyntaxNode reviewed Nov 20, 2024
View reviewed changes
Copy link
Contributor

@SyntaxNode SyntaxNode left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I appreciate your concern for test coverage within this project.

I've reviewed the findings and most of the directories without test coverage are either only data models or a contain a very small abstraction for the benefit of tests where real tests are not practical. I'm not in favor of forcing these tests to exist.

Is there a problem you're looking to solve?

There is already a build action running on all PRs to enforce a minimum test coverage. Further all PRs are reviewed for test coverage before being accepted. You'll see the results of that action above my comment with a link to a visual representation of coverage for easy review.

adapters/adapterstest/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory contains the json test framework for adapters. Perhaps it could be moved up a level.

exchange/entities/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory only contains data models. There is nothing to test.

analytics/clients/http_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory only contains a single line abstraction around the default http client. There is no need for tests.

analytics/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory only contains data models and interfaces. There is nothing to test.

hooks/hookstage/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This could benefit from tests for some types like ChangeSetBids.

stored_requests/caches/cachestest/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory only contains test helper methods There is nothing to test.

stored_requests/caches/nil_cache/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory implements a no-op cache. There is nothing to test.

util/randomutil/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory abstracts a random generator specifically for reliable tests. There is nothing to test.

util/timeutil/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory abstracts a the clock specifically for reliable tests. There is nothing to test.

util/uuidutil/dummy_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

This directory abstracts a random generator specifically for reliable tests. There is nothing to test.

@scr-oath
Copy link
Contributor Author

I appreciate your concern for test coverage within this project.

I've reviewed the findings and most of the directories without test coverage are either only data models or a contain a very small abstraction for the benefit of tests where real tests are not practical. I'm not in favor of forcing these tests to exist.

Is there a problem you're looking to solve?

There is already a build action running on all PRs to enforce a minimum test coverage. Further all PRs are reviewed for test coverage before being accepted. You'll see the results of that action above my comment with a link to a visual representation of coverage for easy review.

How does one run that locally? It might be nice to set up some Tasks for common tasks that self-document what they're for, and which can be run easily both by developers and by CI/CD pipelines.

@scr-oath
Copy link
Contributor Author

scr-oath commented Dec 13, 2024 •
edited
Loading

FWIW, I don't feel strongly enough about this - we have it in our local fork but I can't argue for it - it was merely a zero-tolerance for code without tests no matter how small and it does allow coverage to be visualized, which isn't done when there are zero tests in a directory.

I'm going to close this.

@scr-oath scr-oath closed this Dec 13, 2024
@scr-oath scr-oath deleted the ensure-tests-where-go-code branch December 13, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SyntaxNode SyntaxNode SyntaxNode left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scr-oath @SyntaxNode