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

Change result note key from a string to a list of strings #3254

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

Conversation

happz
Copy link
Collaborator

@happz happz commented Oct 1, 2024

This better represents its real nature: a collection of various notes, there may be more than one note or topic tmt needed to share with user depending on what happened while the test was running.

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • modify the json schema
  • mention the version
  • include a release note

@happz happz added breakage Change will change current behaviour area | results Related to how tmt stores and shares results labels Oct 1, 2024
@happz happz added this to the 1.38 milestone Oct 1, 2024
@happz
Copy link
Collaborator Author

happz commented Oct 1, 2024

@psss @thrix @lukaszachy This is definitely a breaking change for TF, and potentially for other users as well. TF ingests the field and treats it as a string, it will require at least two patches to accommodate the change - simple ones, but still.

Yet I believe this change should happen because it better reflects the nature of the note field: there can be multiple notes attached to a single test, and 1. code needs to take extra measures to stack notes correctly, and 2. this robs consumer from processing them, e.g. filtering or rendering accordingly to their meaning for the end user. For example, a test with a failed test check that failed to reboot its guest & enforces interpretation of its result by setting its result field may collect three notes - and such a scenario is not that unusual, reboots are common, outages too, and checks are getting more and more traction. By using a list, we would make the code simpler, it'd let consumers format and mangle them as they need, and it'd be easier to add new notes - which I think will be a considerable advantage WRT results of prepare and finish steps we don't generate yet, but I experimented a bit with those and some plugins may share extra details in their result notes, which was very tedious with note += ', foo...'.

So, how to proceed to make it as painless for the end user as possible? Do we know who out there consumes results.yaml?

@happz happz added the ci | full test Pull request is ready for the full test execution label Oct 7, 2024
@happz happz force-pushed the results-note-as-array branch 2 times, most recently from 713c0e6 to f15eee5 Compare October 7, 2024 13:05
@thrix
Copy link
Collaborator

thrix commented Oct 8, 2024

So, how to proceed to make it as painless for the end user as possible? Do we know who out there consumes results.yaml?

I am fine introducing the breaking change, makes sense to me. I am not aware of any other user of results.yaml besides Testing Farm. Our users do not consume this file, they consume XML if they need some machine-readable output afaik.

I would like to take this as an example how the upcoming integration test to Testing Farm will provide a value here. Use it as a guinea pig showing that our Testing Farm worker code it is ready for this change.

@thrix
Copy link
Collaborator

thrix commented Oct 10, 2024

So, how to proceed to make it as painless for the end user as possible? Do we know who out there consumes results.yaml?

I am fine introducing the breaking change, makes sense to me. I am not aware of any other user of results.yaml besides Testing Farm. Our users do not consume this file, they consume XML if they need some machine-readable output afaik.

I would like to take this as an example how the upcoming integration test to Testing Farm will provide a value here. Use it as a guinea pig showing that our Testing Farm worker code it is ready for this change.

Still working it out, got blocked a bit :(

@@ -266,7 +266,7 @@ Context:
{% endfor %}
</td>
<td class="data"><a href="{{ base_dir | linkable_path | urlencode }}/{{ result.data_path | urlencode }}">data</a></td>
<td class="note">{% if result.note %}{{ result.note | e }}{% else %}-{% endif %}</td>
<td class="note">{% if result.note %}{{ result.printable_note | e }}{% else %}-{% endif %}</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is printable_note property actually needed? I think we should maybe print the notes as HTML list and keep the display/design of these notes on the Jinja template, e.g.:

{% if result.note %}
<ul>
{% for note in result.note %}
<li>{{ note | e }}</li>
{% endfor %}
</ul>
{% else %}
-
{% endif %}

This better represents its real nature: a collection of various notes,
there may be more than one note or topic tmt needed to share with user
depending on what happened while the test was running.
@thrix thrix modified the milestones: 1.38, 1.39 Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area | results Related to how tmt stores and shares results breakage Change will change current behaviour ci | full test Pull request is ready for the full test execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants