-
Notifications
You must be signed in to change notification settings - Fork 135
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
[issue-487] add tests for the document_utils.py
module
#533
[issue-487] add tests for the document_utils.py
module
#533
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for increasing the test coverage, I think we should also add negative tests here as commented below.
tests/spdx/test_document_utils.py
Outdated
@@ -0,0 +1,27 @@ | |||
# SPDX-FileCopyrightText: Copyright (c) 2023 spdx contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is valid, I would understand the specification to either use Copyright (c) ..
or SPDX-FileCopyrightText: 2023..
. The suggested change would align with the header in test_creation_info_writer.py
.
# SPDX-FileCopyrightText: Copyright (c) 2023 spdx contributors | |
# SPDX-FileCopyrightText: 2023 spdx contributors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed and adapted my copyright profile (again^^)
document, package, file, snippet = variables | ||
assert get_element_from_spdx_id(document, package.spdx_id) == package | ||
assert get_element_from_spdx_id(document, file.spdx_id) == file | ||
assert get_element_from_spdx_id(document, snippet.spdx_id) == snippet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there is no matching element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should return None
, I will add this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
def test_contained_element_ids(variables): | ||
document, package, file, snippet = variables | ||
element_ids = get_contained_spdx_element_ids(document) | ||
TestCase().assertCountEqual(element_ids, [package.spdx_id, file.spdx_id, snippet.spdx_id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also check that no other element is contained? The test relies on the fact that document_fixture
only contains the respective elements from the fixtures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertCountEqual()
already tests that there exactly the same elements (with multiplicity) in the lists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, true, my brain is still on vacation :D
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
6fbe644
to
042e7c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes #487