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

Add indexes to test names so that Specmatic always generates tests with unique names. #1082

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

burdeamit
Copy link
Member

@burdeamit burdeamit commented Apr 29, 2024

What:

At present, Specmatic adds an index suffix to all negative tests with the same name, however this behaviour is not implemented for positive tests. This results in Specmatic generating positive tests with the same name duplicated.
This PR enforces a consistent behaviour so that, whenever a group of tests [ Positive or Negative ] with the same name are generated, an index is suffixed to the test name so that all tests have unique names.

Why:

Duplicate test names result in python wrapper reporting an incorrect test count.

Checklist:

jaydeepk and others added 3 commits April 25, 2024 15:28
…d wih an index so that the test name is always unique.
…scenarios.

Fixed space format issue in index label.
Added test for negative test scenarios.
Copy link
Member

@harikrishnan83 harikrishnan83 left a comment

Choose a reason for hiding this comment

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

Thanks, requires minor changes.

@@ -365,19 +365,13 @@ data class Feature(
val negativeTestScenarios =
negativeScenario.generateTestScenarios(flagsBased, testVariables, testBaseURLs)

negativeTestScenarios.filterNot { negativeTestScenarioR ->
val filtered = negativeTestScenarios.filterNot { negativeTestScenarioR ->
Copy link
Member

Choose a reason for hiding this comment

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

Can we please rename filtered to what it actually contains? This will help readability of line where filtered is being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -365,19 +365,13 @@ data class Feature(
val negativeTestScenarios =
negativeScenario.generateTestScenarios(flagsBased, testVariables, testBaseURLs)

negativeTestScenarios.filterNot { negativeTestScenarioR ->
val filtered = negativeTestScenarios.filterNot { negativeTestScenarioR ->
Copy link
Member

Choose a reason for hiding this comment

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

not sure I understand why we are naming this negativeTestScenarioR? what is the R? Can you please check?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • negativeTestScenarioR is essentially a temporary variable used to hold each individual negative test scenario as it's being evaluated by the filterNot function.
  • This will be renamed to currentNegativeTestScenario. So the name indicates that this is the current negative test scenario being evaluated in the filter operation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -400,6 +394,27 @@ data class Feature(
serverState = emptyMap()
}

private fun addIndexes(
Copy link
Member

Choose a reason for hiding this comment

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

Technically this method is adding both +v/-ve prefix as well as index, should it be named appropriately?

Copy link
Member

Choose a reason for hiding this comment

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

Will also be super helpful to unit test this method separately. While the feature level tests are very useful, this will allow is try all difficult combinations.

Copy link
Member

Choose a reason for hiding this comment

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

Is this method doing too much, should we separate out application of prefix and index as two separate collections.map operations?

Copy link
Member Author

Choose a reason for hiding this comment

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

ToDo's

  • Technically this method adds both the +v/-ve prefix and the index, should it be named appropriately?
  • It will also be super helpful to unit test this method separately. While the feature-level tests are very useful, this will allow us to try all difficult combinations.
  • Is this method doing too much, should we separate the application of prefix and index as two separate collections.map operations?

core/src/test/kotlin/in/specmatic/core/FeatureTest.kt Outdated Show resolved Hide resolved
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