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

Modulemd.ModuleIndexMerger.test_merger_with_real_world_data into C #412

Merged
merged 1 commit into from
Jan 23, 2020
Merged

Modulemd.ModuleIndexMerger.test_merger_with_real_world_data into C #412

merged 1 commit into from
Jan 23, 2020

Conversation

madhavmehndiratta
Copy link
Contributor

@madhavmehndiratta madhavmehndiratta changed the title Copying python tests to C Modulemd.ModuleIndexMerger.test_merger_with_real_world_data into C Dec 28, 2019
Copy link
Member

@OrionStar25 OrionStar25 left a comment

Choose a reason for hiding this comment

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

Great start! Welcome to Modularity :D

You have created a test/function. Now you need this test to run. All execution starts from the main. Hence you need to define your tests there. This will make sure your tests are running ad are correct.
Please look at https://github.com/fedora-modularity/libmodulemd/blob/master/modulemd/tests/test-modulemd-merger.c#L249 to see how other tests of this file have been added.

@@ -273,3 +273,31 @@ main (int argc, char *argv[])

return g_test_run ();
}

test_merger_with_real_world_data (ModuleStreamFixture *fixture,
Copy link
Member

@OrionStar25 OrionStar25 Dec 28, 2019

Choose a reason for hiding this comment

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

Every function in C needs a return type unlike in Python. Here the return type would be static void. This would throw you a compilation error when running ninja

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the file again and fixed some errors.

Copy link
Member

Choose a reason for hiding this comment

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

Please push your changes to this PR so that I can see them.

Copy link
Member

@OrionStar25 OrionStar25 left a comment

Choose a reason for hiding this comment

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

Please run ninja test and submit again :)

@@ -360,7 +360,7 @@ modulemd_validate_nevra (const gchar *nevra)
{
break;
}
else if (*i == '-')
if (*i == '-')
Copy link
Member

@OrionStar25 OrionStar25 Dec 29, 2019

Choose a reason for hiding this comment

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

Why this? Put it back to original please.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The code formatter that we run during ninja test recently started to drop unneeded else statements. It's unrelated to this change. I've pushed this to the current master branch so it doesn't keep showing up.

Copy link
Member

@OrionStar25 OrionStar25 left a comment

Choose a reason for hiding this comment

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

ninja test is behaving weirdly for this. I'll approve your task for now. Good work!

Copy link
Collaborator

@sgallagher sgallagher left a comment

Choose a reason for hiding this comment

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

Please rebase atop the latest master branch and squash your patches into a single one.

Copy link
Collaborator

@sgallagher sgallagher left a comment

Choose a reason for hiding this comment

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

Tests are still failing because you didn't run ninja test before pushing the latest version. The g_test_add_func () line is more than 79 characters long, which means that the code-reformatting tool will rewrite it during ninja test.

Please run ninja test and then amend the patch with the fixes.

@OrionStar25
Copy link
Member

@Mr-M1M3 Please rebase and run ninja test before committing.

Modulemd.ModuleIndexMerger.test_merger_with_real_world_data into C
@madhavmehndiratta
Copy link
Contributor Author

I ran ninja test before commiting the changes, still the error exists

sgallagher added a commit that referenced this pull request Jan 23, 2020
Modulemd.ModuleIndexMerger.test_merger_with_real_world_data into C

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
@sgallagher sgallagher merged commit 0ace3b5 into fedora-modularity:master Jan 23, 2020
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.

None yet

3 participants