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

Phg/query params disamb #1127

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

Phg/query params disamb #1127

wants to merge 5 commits into from

Conversation

Pgarrett
Copy link
Collaborator

@Pgarrett Pgarrett commented Nov 8, 2024

  • Adding an extra layer for test naming differentiation for REST scenarios. It takes into account the presence of query params, but not the values. The latter ones will be added for the experimental features, but setting the grounds here.
  • Test disambiguation now takes place until no change has been detected. This will happen since the check runs against the size of duplicatedIndividuals: MutableSet, either no more disambiguation takes place (no individuals are removed from the set) or eventually all individuals are disambigauted (the set size is now 0 and the next iteration will have the same size)

* The filter call ensures that we are only performing this disambiguation when there's only one individual that
* differs and the list of query params is not empty.
*/
private fun checkForQueryParams(duplicatedIndividuals: MutableSet<EvaluatedIndividual<*>>): Map<EvaluatedIndividual<*>, String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess could be specified <RestIndividual instead of <*>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried doing so, the Kotlin type system didn't like it much

.filter { it.value.size == 1 && it.key.isNotEmpty()}
.mapNotNull { entry ->
val eInd = entry.value[0]
duplicatedIndividuals.remove(eInd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this? this is a side effect on the input list. quite confusing, considering that the name of the function is check. also, this is the same datastructure used on line 118 at the beginning of stream operations (althugh these ones are eagerly evaluated), which complicates the understanding of the function. this needs refactoring/simplification

Copy link
Collaborator

Choose a reason for hiding this comment

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

as rule of thumb, side effects on inputs data structures should be avoided, unless explicitely clarified in the method name (somehow)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just in case, it follows the same pattern as the path disambiguation in the previous PR. It's also clarified in the Javadoc of the top method.

Still, I'll work on refactor, simplification and naming

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've just pushed a draft on a possible solution. It transfers the responsibility of removing the solved duplicates to a specific function.
The downside is the duplicate removal happens twice:

  1. in the resolveAmbiguities function where the local working copy has the resolved duplicates removed
  2. in the outer loop which triggers the resolution in NumberedTestCaseNamingStrategy.

However, this downside could be an advantage if we decide that all ambiguity solvers should be executed for resolution. It could even be tested for the experimentation: "do you like this naming where it only solves by one particular parameter or do you want the whole enchilada?".

Let me know what you think. Once we decide on this I'll also extract the groupBy sections of the solvers to more clearly named methods

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.

2 participants