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

[happz]

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]

@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?

[thrix]

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]

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 :(

[martinhoyer]

@happz Thanks for this!
Would this be a breaking change for custom-results users?

[psss]

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 :(

@thrix, is the Testing Farm part ready for this change? Or shall we still postpone merging?

[janhavlin]

I created a patch to account for this change on TF side: https://gitlab.com/testing-farm/gluetool-modules/-/merge_requests/856

TF will accept list[str] | str | None type in the note field.

[psss]

I created a patch to account for this change on TF side

Great, thanks! Then we are unblocked here, rebased.

[psss]

Looks good, I just find the list format in the html report not well aligned. Propose a slight css style adjustmetn in 20a55f1.

For comparison, this is before the change:

After the change:

And with the style adjustment:

[psss]

@happz, seems that unit tests will need some adjustments as well:

>       assert interpreted.note is None
E       AssertionError: assert [] is None

[happz]

@happz, seems that unit tests will need some adjustments as well:

>       assert interpreted.note is None
E       AssertionError: assert [] is None

Hopefully addressed in f705e9e.

[psss]

Hopefully addressed in f705e9e.

Great, thanks! Tests are now green.

I see three checklist items missing and I'd say they all make sense. What about adding a short mention about the note field change from string to lint to the result specification and mention the tmt version there? Probably worth also to mention this in the release notes. @happz, what say you?

[happz]

Hopefully addressed in f705e9e.

Great, thanks! Tests are now green.

I see three checklist items missing and I'd say they all make sense. What about adding a short mention about the note field change from string to lint to the result specification and mention the tmt version there? Probably worth also to mention this in the release notes. @happz, what say you?

Sure, how about cfa5e89?

[psss]

Sure, how about cfa5e89?

Great! And we're done here, thanks!