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

Remove ineffective tests #2209

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Remove ineffective tests #2209

merged 1 commit into from
Jan 18, 2024

Conversation

vy
Copy link
Member

@vy vy commented Jan 17, 2024

This PR either fixes or removes ineffective tests.

@vy vy added the tests Pull requests or issues related to tests label Jan 17, 2024
@vy vy added this to the 3.0.0 milestone Jan 17, 2024
@vy vy requested a review from ppkarwasz January 17, 2024 15:02
@vy vy self-assigned this Jan 17, 2024
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

I'm surprised at how much cruft accumulated in the test code!

@vy vy merged commit 8ae25b9 into main Jan 18, 2024
9 checks passed
@vy vy deleted the main-remove-ineffective-tests branch January 18, 2024 12:51
@rgoers
Copy link
Member

rgoers commented Jan 23, 2024

What criteria was used to determine that the tests were "ineffective"? Anything with "perf" in the name was just deemed ineffective?

@vy
Copy link
Member Author

vy commented Jan 23, 2024

What criteria was used to determine that the tests were "ineffective"? Anything with "perf" in the name was just deemed ineffective?

@rgoers, removed tests either were disabled long ago (causing compilation time waste) or contained no assertions (causing compilation + testing time waste). I tried saving whatever I can and removed the rest of such ineffective tests.

@rgoers
Copy link
Member

rgoers commented Jan 23, 2024

@vy There were a couple of "tests" you removed that did not have any assertions that I used for occaisional sanity checks. Namely FilterPerformanceComparison, SimplePerfTest, and ThreadedPerfTest. I never found a way to assert what I wanted to check - namely that someone hadn't inserted code that seriously impacted the performance of what those are doing- SimplePerfTest primarily validates that the cost of logging is close to zero when disabled. FilterPerformanceComparison ensures we haven't added something that causes filtering to slow down - comparing it to Logback is a decent way to do that. One of the main issues is the performance varies from machine to machine so it is very tough to make assertions. Instead, I just eyeball the results based on what I expect on my machine.

@vy
Copy link
Member Author

vy commented Jan 23, 2024

@rgoers, thanks for sharing the context, now I understand. These use cases weren't apparent to me from the code. I am was actively in the search of finding the right way to establish a continuous (and reproducible!) performance test bed. The checks you mentioned indeed serve an important purpose an should ideally be a part of the build. I will check if we can flesh out an attack plan. I will first consult to INFRA if we can get our hands on a machine where we can run performance tests without external noise and, ideally, manage it in GitHub Actions so that we can make it a part of our CI pipeline.

@ppkarwasz ppkarwasz modified the milestones: 3.0.0, 3.0.0-beta2 Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Pull requests or issues related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants