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

[issue-394] add tests for tag_value writer #567

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

meretp
Copy link
Collaborator

@meretp meretp commented Apr 6, 2023

fixes #394

Copy link
Collaborator

@armintaenzertng armintaenzertng left a comment

Choose a reason for hiding this comment

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

Thanks for the effort, much appreciated! :)
However, I think it would be in our best interest to replace all of the value strings with references to the fixture values, at least where possible.
For example, "FileName: ./fileName.py\n" would become f"FileName: {file_fixture().name}\n"

@meretp
Copy link
Collaborator Author

meretp commented Apr 6, 2023

You are absolutely right, I replaced most strings. I didn't replace date strings and other strings that have a bit more logic than just a reference to the fixture value, please have another look.

armintaenzertng
armintaenzertng previously approved these changes Apr 6, 2023
Copy link
Collaborator

@armintaenzertng armintaenzertng left a comment

Choose a reason for hiding this comment

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

Thanks, no big issues from my side!

tests/spdx/writer/tagvalue/test_file_writer.py Outdated Show resolved Hide resolved

mock.assert_called_once_with("foo", "w")
handle = mock()
handle.write.assert_has_calls(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method allows other calls before or after the given list of calls. I haven't found a way to check that there are no such calls but maybe this isn't too important to check right now, anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, another option would be assert_has_calls but this wouldn't check the order of calls, which is more important I think than the "risk" of calls before or after this statement.

Copy link
Collaborator

@armintaenzertng armintaenzertng left a comment

Choose a reason for hiding this comment

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

Thanks! :)

@meretp meretp merged commit 01b8a75 into spdx:main Apr 11, 2023
@meretp meretp deleted the issue_394_add_tests_for_tag_value_writer branch April 11, 2023 06:36
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.

Add tests for tag-value writer
2 participants