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-573] unify parse_from_file tests for all formats #574

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

armintaenzertng
Copy link
Collaborator

Fixes #573.
I went a little further and unified the general parsing tests for all formats into a single test class, also adding the v2.2 tag-value example.

@armintaenzertng armintaenzertng force-pushed the unifyParseFromFileTests branch 3 times, most recently from 54bfc9d to 83ff11a Compare April 12, 2023 11:45
meretp
meretp previously approved these changes Apr 12, 2023
Copy link
Collaborator

@meretp meretp left a comment

Choose a reason for hiding this comment

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

Thanks! Two comments from my side, that are rather optional.

class TestParseFromFile:
def test_parse_from_file_not_found(self, parser, format_name, extension):
with pytest.raises(FileNotFoundError) as err:
wrong_file_path = os.path.join(os.path.dirname(__file__), "hnjfkjsedhnflsiafg.json")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use extension here. Doesn't make a difference but feels more consistent regarding the different parsers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This feels necessary and unnecessary at the same time :D
I'll add it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if we really need the folder formats. Right now this gives an additional depth, that is not really needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same thought. I'll remove it for now.

Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
Signed-off-by: Armin Tänzer <armin.taenzer@tngtech.com>
@armintaenzertng armintaenzertng force-pushed the unifyParseFromFileTests branch from 0a28990 to 8ae5a89 Compare April 12, 2023 13:50
Copy link
Collaborator

@meretp meretp left a comment

Choose a reason for hiding this comment

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

LGTM!

@armintaenzertng armintaenzertng merged commit 0138e9c into spdx:main Apr 13, 2023
@armintaenzertng armintaenzertng deleted the unifyParseFromFileTests branch April 13, 2023 06:10
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 yaml and xml parser tests
2 participants