search

chaoss / grimoirelab-graal

A Generic Repository AnALyzer
GNU General Public License v3.0
21 stars 62 forks source link

Fix unit tests that mistakenly used assertTrue(a,b) instead of assertEqual(a,b) #103

Open jwalden opened 2 years ago

jwalden commented 2 years ago

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.

valeriocos commented 2 years ago

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!

jwalden commented 2 years ago

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 https://github.com/chaoss/grimoirelab-graal/pull/103#issuecomment-1011858271, 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: @.***>

valeriocos commented 2 years ago

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). 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

jwalden commented 2 years ago

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 https://github.com/chaoss/grimoirelab-graal/pull/103#issuecomment-1012375211, 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: @.***>

vchrombie commented 2 years ago

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.

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!!

jwalden commented 2 years ago

I updated the commit message a couple of weeks ago. Are there any other changes needed to accept this PR?

vchrombie commented 2 years ago

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 https://github.com/chaoss/grimoirelab-graal/issues/109 for this purpose. I will follow up on this and make sure to merge this PR once it is ready.

vchrombie commented 2 years ago

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.