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

Support multiple test contacts in the ReportPortal plugin #3412

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

Conversation

seberm
Copy link
Collaborator

@seberm seberm commented Dec 8, 2024

Add the possibility of exporting multiple test contacts into the report portal instance.

Closes: #3407

TODO:

  • Fix the local reportportal "contact" attribute test

Pull Request Checklist

  • implement the feature
  • extend the test coverage
  • include a release note

@seberm seberm added the plugin | reportportal The reportportal report plugin label Dec 8, 2024
@seberm seberm added this to the 1.41 milestone Dec 8, 2024
@seberm seberm linked an issue Dec 8, 2024 that may be closed by this pull request
@psss psss force-pushed the feature/reportportal-improvements branch from db5fbd6 to 5cac74d Compare December 9, 2024 15:04
Base automatically changed from feature/reportportal-improvements to main December 9, 2024 17:07
@seberm seberm force-pushed the feature/reportportal-multiple-contacts branch 2 times, most recently from fb8c975 to f951c36 Compare December 11, 2024 15:03
@seberm seberm marked this pull request as ready for review December 11, 2024 15:12
@happz happz added the ci | full test Pull request is ready for the full test execution label Dec 11, 2024
@seberm
Copy link
Collaborator Author

seberm commented Dec 11, 2024

Hello,
under which "key" do we want to save these contacts in the ReportPortal?

The contacts are always saved under the contact field of a specific test item with this patch. The attributes attached to the test item look the following:

[
  ...
  { "contact": "[email protected]" },
  { "contact": "[email protected]" },
  ...
]

Another option is to enumerate each contact key:

[
  ...
  { "contact_0": "[email protected]" },
  { "contact_1": "[email protected]" },
  ...
]

Any preferences? Thanks!

@seberm seberm changed the title Add possibility to export multiple test contacts into the ReportPortal Add the possibility to export multiple test contacts into the ReportPortal Dec 17, 2024
@seberm
Copy link
Collaborator Author

seberm commented Dec 17, 2024

We decided we will go with the first option:

[
  ...
  { "contact": "[email protected]" },
  { "contact": "[email protected]" },
  ...
]

This PR is ready for review.

Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@psss psss force-pushed the feature/reportportal-multiple-contacts branch from f951c36 to 7110dcf Compare December 17, 2024 13:18
@psss psss requested a review from martinhoyer as a code owner December 17, 2024 13:18
@psss psss changed the title Add the possibility to export multiple test contacts into the ReportPortal Support multiple test contacts in the ReportPortal plugin Dec 17, 2024
@psss
Copy link
Collaborator

psss commented Dec 17, 2024

A short release note would be nice.

@seberm
Copy link
Collaborator Author

seberm commented Dec 17, 2024

A short release note would be nice.

@psss Added, thanks for the review!

@psss psss force-pushed the feature/reportportal-multiple-contacts branch from 6adb5ff to 10dc3a9 Compare December 18, 2024 08:58
@psss psss added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Dec 18, 2024
@psss
Copy link
Collaborator

psss commented Dec 18, 2024

Hm, /tests/finish/prune started to fail for no obvious reason with:

:: [ 09:13:23 ] :: [   FAIL   ] :: File /tmp/tmp.5qqARmNv2o/plan/execute/data/guest/default-0/default-2/write/test-data-3/data/out-test.txt should exist 

This does not seem to be related to this change. When executed locally, everything is just fine 😵‍💫

@lukaszachy
Copy link
Collaborator

lukaszachy commented Dec 18, 2024

@psss fails for me locally as well. I make rpm current content, installed (just tmt main package) into fedora:41 container and executed the tests/finish/prune/test.sh

It fails because that asserted file exists as test-data-4/data/out-test.txt, not test-data-3

Fastest way to check is (failing version)

$ cd tests/finish/prune/data
$ tmt run discover -vvv
        summary: 4 tests selected
            /default-0/test/test1
            /default-1/test/test2
            /default-2/write/plan-data
            /default-2/write/test-data

Passing version has the last two tests in switched order.

@happz
Copy link
Collaborator

happz commented Dec 18, 2024

@psss fails for me locally as well. I make rpm current content, installed (just tmt main package) into fedora:41 container and executed the tests/finish/prune/test.sh

It fails because that asserted file exists as test-data-4/data/out-test.txt, not test-data-3

Fastest way to check is (failing version)

$ cd tests/finish/prune/data
$ tmt run discover -vvv
        summary: 4 tests selected
            /default-0/test/test1
            /default-1/test/test2
            /default-2/write/plan-data
            /default-2/write/test-data

Passing version has the last two tests in switched order.

The problem with the changed order of tests has been mentioned this evening on tmt Slack channel, this seems like the same issue. I suspect fmf library is a culprit here, but I didn't follow fmf development closely enough for me to point a finger at a commit (and no tmt change rings a bell). But it should be easy to check, the test would pass with tmt & an older fmf version.

shell plugin takes the list of tests, constructs an fmf tree out of them, and populates internal Test instances from this tree - and if there was a change sorting of fmf nodes, items, keys, that might have affected this process

@lukaszachy
Copy link
Collaborator

lukaszachy commented Dec 19, 2024

@happz @psss Yes indeed, fmf-1.4.1 vs 1.5.0 return different order of tests.

EDIT: fmf behavior changed in teemtee/fmf#260, it's the 'sort' added in https://github.com/teemtee/fmf/blob/95c9745db00b0e2b545a8e93021544c10d8665ac/fmf/base.py#L714

@happz
Copy link
Collaborator

happz commented Dec 19, 2024

@happz @psss Yes indeed, fmf-1.4.1 vs 1.5.0 return different order of tests.

Which sounds like a breaking change in case of lists, as demonstrated by the shell disover plugin, where the order of tests was apparently given by their order in the list of tests.

@psss
Copy link
Collaborator

psss commented Dec 19, 2024

So the problem is that previously the data key sorting happened when the fmf.Tree was being built and now we sort the keys when the Tree.climb() method is called.

https://github.com/teemtee/fmf/blob/95c9745db00b0e2b545a8e93021544c10d8665ac/fmf/base.py#L710-L716

The change does not affect trees which are built directly from fmf files on disk because those were always sorted by node name. However, when the Tree.child() method is used for creating child nodes manually, those nodes were kept in the original order as they were created. This is what the shell discover plugin uses:

test_fmf_keys: dict[str, Any] = {
key: value
for key, value in data.to_spec().items()
if key != 'name' and (key == 'duration' or value != data.default(key))
}
tests.child(data.name, test_fmf_keys)

So these nodes are now sorted as well, which for this specific use case probably does not make sense: User provided the tests in the form of a list and they most probably expect the order should be preserved.

I see two possible ways to fix this:

  • do not sort keys when climbing the Tree
  • make the sorting optional / configurable

If we go the first way, it would break tests which rely on the sorting. And I guess this approach can be quite widely used. So, let's make it configurable? Or any other/better ideas?

@lukaszachy
Copy link
Collaborator

make the sorting optional / configurable

I'd say it should be configurable. Caller of climb method should know if they want to keep the order in which they created the data (eg. as this discover.shell) or sorted (majority of other use cases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution plugin | reportportal The reportportal report plugin status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store all contact emails when uploading results in reportportal
5 participants