Fix unit tests that mistakenly used assertTrue(a,b) instead of assertEqual(a,b) #103
Conversation
compare the values a and b with assertEqual(a,b) which does compare the two values. Signed-off-by: James Walden <[email protected]>
|
Hi @jwalden, thank you for your contribution. Since the two commits are addressing different things (a fix in the tests and a new analyzer), I'd suggest to submit 2 PRs. This would simplify the reviewing process, thanks! |
|
I'm new to PRs, so I'm not sure what went wrong. I submitted a PR with only
one commit in it, the one fixing the unit tests.
Later that day, I pushed a commit with my work for the day to my forked
repository on GitHub. I did not submit a PR for this commit, as it's
incomplete.
How are you seeing the second commit I made after submitting the PR?
…On Thu, Jan 13, 2022 at 2:12 AM valerio ***@***.***> wrote:
Hi @jwalden <https://github.com/jwalden>, thank you for your
contribution. Since the two commits are addressing different things (a fix
in the tests and a new analyzer), I'd suggest to submit 2 PRs. This would
simplify the reviewing process, thanks!
—
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB43GKZJQJCWBGKBTBESXTUVZ3OZANCNFSM5LZMTNHA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Your branch is now visible through this PR, thus any change you commit there will be automatically reflected here. The tab To fix the DCO failure, please remember to sign-off your commits, details here: https://github.com/chaoss/grimoirelab/blob/master/CONTRIBUTING.md#dco-and-sign-off-for-contributions, thanks |
|
I think I've fixed this. I moved the second commit to the cqmetrics branch.
Let me know if you still see both commits or just one.
…On Thu, Jan 13, 2022 at 12:58 PM valerio ***@***.***> wrote:
Your branch is now visible through this PR, thus any change you commit
there will be automatically reflected here. The tab commits in this page
lists the commits you have pushed to your branch and are not present in
master (some additional info is available here
<https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request>).
Hope this helps.
To fix the DCO failure, please remember to sign-off your commits, details
here:
https://github.com/chaoss/grimoirelab/blob/master/CONTRIBUTING.md#dco-and-sign-off-for-contributions,
thanks
—
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB43GKHF4EXP3LSUAUQYG3UV4HDPANCNFSM5LZMTNHA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes, I think this is fixed. I don't see any unrelated change in the PR. But, I think the commit message needs to be improved. You can find the guidelines to write a proper commit message from https://github.com/chaoss/grimoirelab/blob/master/CONTRIBUTING.md#guidelines-to-follow-to-write-good-commit-messages Please let me know if you need any help related to it. Thanks for your contribution!! |
Many unit tests used assertTrue(a,b) to test for equality of a and b. To test equality of a and b in PyUnit, assertEqual(a,b) should be used, so this commit replaces occurrences of assertTrue() with assertEqual() when comparison is intended. Signed-off-by: James Walden <[email protected]>
|
I updated the commit message a couple of weeks ago. Are there any other changes needed to accept this PR? |
Sorry for the inconvenience caused @jwalden. I missed the PR. The builds were failing because of the old python dependency. I have opened #109 for this purpose. |
|
Hi @jwalden, sorry for making you wait for so long. We have fixed the CI and other issues. Can you please rebase your branch to include the latest changes and test the workflows too. Thank you. Please let me know if you need any help. |
In unit tests, replaced occurrences of assertTrue(a,b) which does not compare the values a and b with assertEqual(a,b) which does compare the two values.