-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add tests for ModulemdModuleStream description #407
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.
Everything else looks good! Thank you :D
@@ -21,6 +21,9 @@ | |||
"À϶¥🌭∮⇒⇔¬β∀₂⌀ıəˈ⍳⍴V)" \ | |||
"═€ίζησθლბშიнстемองจึองታሽ።ደለᚢᛞᚦᚹ⠳⠞⠊⠎▉▒▒▓😃" | |||
|
|||
#define MMD_TEST_DESC_LOCALE "C" |
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.
I don't think this is necessary as "C" refers to the default locale. It's unlikely to change. I personally think the readability is being lost by such a huge macro, but I think we'll let this stay.
@sgallagher What do you think?
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.
So should I just write "C" as an argument rather then create macro?
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.
I also forgot an important case that was discussed last time.
Please also add a test/assertion in the case when the description contains any kind of Unicode. The macro for it is already defined. MMD_TEST_DOC_UNICODE_TEXT
.
Please look at the documentation
tests to see how it has been done.
Added that. |
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.
The output from the failed tests:
92/94 test_dirty_repo FAIL 0.28 s (exit status 64)
--- command ---
/usr/bin/python3 /builddir/travis/../modulemd/tests/test-dirty.py
--- stdout ---
Autoformatter was not run before submitting. Please run `ninja test`, amend the commit and resubmit this pull request.
@Norem80 your CI tests are failing because of linting issue. Please always run ninja test
before committing. This also enables proper linting for the entire codebase before submission. I would also like you to squash all of your commits into one single commit. We always aim for 1 commit per 1 logical change. In this case, 1 commit for 1 added test.
There are conflicts with the current master branch. Would you mind doing a |
@Norem80 Please rebase this pull request. |
Add tests for ModulemdModuleStream description Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
Copied python tests for Modulemd.ModuleStream.description into C as a Google Code-in 2019 Task.