Skip to content

Change read visibility of PickleId property to public.#1050

Draft
clrudolphi wants to merge 2 commits intomainfrom
Expose_PickleId_as_Public
Draft

Change read visibility of PickleId property to public.#1050
clrudolphi wants to merge 2 commits intomainfrom
Expose_PickleId_as_Public

Conversation

@clrudolphi
Copy link
Contributor

🤔 What's changed?

Change visibility of PickleId property to public.

⚡️ What's your motivation?

See discussion #1049

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

I don't see a need to add tests for this simple change.

📋 Checklist:

  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

@304NotModified
Copy link
Member

304NotModified commented Feb 26, 2026

I don't see a need to add tests for this simple change.

Agree. If we like to see some API backwards compatiblility tests, just let me know.

/// This holds the unique identifier for the test ("pickle") represented by this scenario info. Used internally at runtime.
/// </summary>
internal string PickleId { get; set; }
public string PickleId { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I havent read the full discussion. But is also a public setter needed?

@clrudolphi
Copy link
Contributor Author

clrudolphi commented Feb 26, 2026 via email

Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

LGTM

@304NotModified 304NotModified changed the title Change visibility of PickleId property to public. Change read visibility of PickleId property to public. Feb 26, 2026

## Bug fixes:
* Fix: Formatters incorrectly handle Unicode text file content of attachments.
* Enhancement: Changed visibility of PickleId property of ScenarioInfo to public.
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be under Improvements by the way. (And maybe mention "read")

@gasparnagy
Copy link
Contributor

I am generally fine with this enhancement. But before we merge, let me just raise my concerns. Maybe I'm a bit confused.

Two problems:

  1. The ScenarioInfo is an information about the scenario. So far we have abused it to also contain information about the pickles, but as long that was internal, it did not break the domain model. Are we sure we would like to expose the pickle id in the ScenarioInfo class? Somewhat I would feel more safe to expose it either in the ScenarioContext (that is essentially the scenario execution context, so pickle id is fine there), or make a PikcleInfo or PickleContext that is accessible from the ScenarioContext (similarly to StepContext)

  2. In the messages overview (https://github.com/cucumber/messages?tab=readme-ov-file#message-overview) it seems that there are three levels: the scenario level, that is the original form, the pickle level - where scenario outlines have been instantiated to individual pickles by examples, and test case level, that is about the pickle executions. If I understand Access ScenarioInfo.PickleIdIndex (or equivalent) #1049 correctly, they would need an identifier that identifies the execution, but pickle id is not that. (The difference is only shown in case of the pickles are being re-run.) Do we have an identifier that identifies the test case execution (test_case_id)?

@clrudolphi
Copy link
Contributor Author

Do we have an identifier that identifies the test case execution (test_case_id)?

Good points. Let me look into this further.

There is a testCaseId, and it might be a more suitable id to use as it would be unique per pickle execution (aka unique per retry).

In terms of where to surface runtime information, you prefer that it be in the Scenario Context right?

@clrudolphi
Copy link
Contributor Author

I realize, now, that this approach has the distinct disadvantage that it won't work for those solutions which are not using Formatters. The PickleId is populated by one of the execution event handlers of the Formatters sub-system.
Let's put this on hold for now until I have further convo with the requesting user.

@clrudolphi clrudolphi marked this pull request as draft February 27, 2026 20:35
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