Skip to content

Conversation

@ramir-dn
Copy link
Contributor

@ramir-dn ramir-dn commented May 8, 2025

We are having an issue where parameterized tests are not grouped together to the same retry group.
It happens for us because the param values are determined at run time and if the retry happens on a different setup, the params might have different values.

The solution we came up with, is to create a test case id that isn't depending on the params values, only on their indices which are the same at all runs even if the values are changed.

This is behavior is configurable in the tc_id marker level, with a new use_index=True param (default is False)

@ramir-dn
Copy link
Contributor Author

ramir-dn commented May 8, 2025

Hello @HardNorth , will be great if you could take a look at this PR, thanks!

@ramir-dn
Copy link
Contributor Author

Hi @HardNorth, can you yell when will you he able yo look at this? Apologize for nagging

@HardNorth
Copy link
Member

@ramir-dn The CI build is failing, please fix.

@HardNorth HardNorth requested a review from Copilot May 15, 2025 10:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new configuration option “rp_ignore_param_val” to generate test case IDs based solely on parameter indices rather than their runtime values, ensuring consistent retry grouping for parameterized tests.

  • Added a new ini option and configuration variable “rp_ignore_param_val”.
  • Updated test and service logic to support generating test case IDs using parameter indices.
  • Introduced helper method _get_parameters_indices in the service layer.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/unit/test_plugin.py Added check to ensure the new "rp_ignore_param_val" option is registered.
pytest_reportportal/service.py Modified test case ID generation to optionally use parameter indices.
pytest_reportportal/plugin.py Added new ini option for "rp_ignore_param_val".
pytest_reportportal/config.py Updated configuration to include "rp_ignore_param_val".
Comments suppressed due to low confidence (1)

tests/unit/test_plugin.py:348

  • Consider adding or updating tests to explicitly cover the behavior when rp_ignore_param_val is set to true, verifying that the test case IDs are generated using only parameter indices.
"rp_ignore_param_val",

@ramir-dn ramir-dn force-pushed the ramir_test branch 2 times, most recently from 0cd82e7 to 3e76f96 Compare May 15, 2025 15:59
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2025

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.71%. Comparing base (ffbeecb) to head (fb7c883).

Files with missing lines Patch % Lines
pytest_reportportal/service.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #398      +/-   ##
===========================================
+ Coverage    69.50%   69.71%   +0.21%     
===========================================
  Files            6        6              
  Lines         1364     1377      +13     
===========================================
+ Hits           948      960      +12     
- Misses         416      417       +1     
Flag Coverage Δ
unittests 69.71% <92.85%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ramir-dn
Copy link
Contributor Author

@HardNorth , fixed, if you can take a look again. Thanks!

@ramir-dn
Copy link
Contributor Author

@HardNorth hello. checking if you will be able to review this sometime soon. thanks!

@HardNorth
Copy link
Member

@ramir-dn Unfortunately, I don't really like your approach. Each test can have it's own params, which will make rp_ignore_param_val property grow and literally inconvenient. I would better handle this in the tc_id mark, in _get_test_case_id method here:
https://github.com/reportportal/agent-python-pytest/blob/develop/pytest_reportportal/service.py#L600

This will give an option to customize parameters for each particular test.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces an option to generate test case IDs that ignore parameter values by basing the IDs on parameter indices instead. Key changes include adding a new flag (use_index) to control test case ID generation, retrieving parameters_indices from the item, and updating test case ID processing accordingly.

Comments suppressed due to low confidence (1)

pytest_reportportal/service.py:609

  • [nitpick] The new flag is named 'use_index' here but the PR description mentions 'rp_ignore_param_val'. Consider renaming the flag to 'rp_ignore_param_val' for consistency.
use_index = mark.kwargs.get("use_index", False)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a configurable option to use parameter indices rather than values when building test case IDs, ensuring stable IDs across runs.

  • Introduce use_index flag on the tc marker to switch ID generation to indices.
  • Capture parameter indices via a new _get_parameters_indices helper and include them in the leaf context.
  • Update _get_test_case_id logic to conditionally pull from parameters_indices.
Comments suppressed due to low confidence (2)

pytest_reportportal/service.py:600

  • The docstring for _get_test_case_id should be updated to describe the new use_index parameter and its effect on ID generation.
def _get_test_case_id(self, mark, leaf: Dict[str, Any]) -> str:

pytest_reportportal/service.py:605

  • There are no tests covering the use_index=True branches. Add unit tests to verify both selected_params and non-selected_params index-based ID generation.
use_index = False

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces an option to generate test case IDs based on parameter indices rather than their runtime values, ensuring consistent grouping for retries.

  • Adds a use_index flag to the tc marker to toggle index-based ID generation.
  • Captures parameter indices via a new _get_parameters_indices helper.
  • Extends _get_test_case_id logic to branch on use_index when building the ID string.
Comments suppressed due to low confidence (2)

pytest_reportportal/service.py:609

  • There are no tests covering use_index=True behavior. Add tests to verify that test case IDs use indices when this flag is set.
use_index = mark.kwargs.get("use_index", False)

pytest_reportportal/service.py:600

  • The method lacks a docstring describing the new use_index parameter and its effect on ID generation. Consider adding documentation for this flag.
def _get_test_case_id(self, mark, leaf: Dict[str, Any]) -> str:

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Add a new use_index flag to generate test case IDs based on parameter indices rather than values, so retries group consistently even if values change.

  • Introduced parameters_indices extraction and storage in test metadata
  • Added use_index marker support in _get_test_case_id to switch between values and indices
  • Added a helper _get_parameters_indices and wired it into metadata population
Comments suppressed due to low confidence (1)

pytest_reportportal/service.py:616

  • Add unit or integration tests covering the use_index=True path in _get_test_case_id, including both when selected_params is provided and when it's not, to validate the new logic.
if use_index:

@ramir-dn
Copy link
Contributor Author

@HardNorth , in our case, we need it for all test cases, this is why I added it globally. But I get you point and I refactored the code.
Now this behavior is controlled by tc_id marker using a new param use_index=True (which is default to false).
We will add tc_id markers to all our tests.
Thanks!

@ramir-dn
Copy link
Contributor Author

@HardNorth , if you can take a look again. Thanks!

@ramir-dn
Copy link
Contributor Author

ramir-dn commented Jun 3, 2025

Hi @HardNorth , any chance you look into this PR soon? Thank you!

@ramir-dn
Copy link
Contributor Author

@HardNorth , apologies for nagging, but really need this fix, if you can take a look 🙏

@HardNorth
Copy link
Member

@ramir-dn
Copy link
Contributor Author

@HardNorth , test cases added in a separate commit. Looking forward to your response, thanks!

@HardNorth HardNorth merged commit 88ae640 into reportportal:develop Jun 12, 2025
6 checks passed
@ramir-dn
Copy link
Contributor Author

Thanks for merge @HardNorth !
Can you also issue a new release for this repo soon?

@HardNorth
Copy link
Member

@ramir-dn Already here: https://github.com/reportportal/agent-python-pytest/releases/tag/5.5.1

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.

3 participants