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

PROC-1475: Suppress 100% allocations #164

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zacharygoodwin
Copy link
Contributor

No description provided.

@zacharygoodwin zacharygoodwin self-assigned this Apr 24, 2024
Copy link
Collaborator

@jamesallen-indeed jamesallen-indeed left a comment

Choose a reason for hiding this comment

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

LGTM; just a couple nits

@@ -454,6 +456,25 @@ protected final void appendTestGroupsWithAllocations(
}
}

private boolean filterRolledOutAllocations(final String testName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'm terrible with names, but I'd prefer this to indicate the return type of boolean. Something like isTestRolledOut

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about this more. isTestRolledOut is not actually descriptive of what this method is doing. It is checking if the test should be logged when the Allocation is fully rolled out to 100%. A better name might be something like shouldLogRolledOutTest or loggableRolledOutAllocation. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. I tend to name methods more for their content than where they're used. So, I like the second option more since it indicates a boolean and what it is doing. Maybe even isRolledOutAllocation. Either way, I trust your judgement, so I'll leave it up to you.

@rishiraj88
Copy link

LGTM for naming conventions.

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.

None yet

3 participants