Changed check if patch info should be in blamelist #1542

Merged
merged 3 commits into from May 3, 2015

Conversation

Projects
None yet
4 participants
@emoon
Contributor

emoon commented Feb 13, 2015

When patch_info is being setup in build_request.py it's being set to (None, None) which makes this check incorrect but patch is being set to None. Now the check patch instead and that resolves the issue outlined in this ticket:

http://trac.buildbot.net/ticket/3194

Changed check if patch info should be in blamelist
When patch_info is being setup in build_request.py it's being set to (None, None) which makes this check incorrect but patch is being set to None. Now the check patch instead and that resolves the issue outlined in this ticket:

http://trac.buildbot.net/ticket/3194
@emoon

This comment has been minimized.

Show comment
Hide comment
@emoon

emoon Feb 13, 2015

Contributor

So it seems that the unit-tests fails which this change:

The coded in build_request.py looks like this when setting up the patch data:

            if ssdata['patch']:
                patch = ssdata['patch']
                ss.patch = (patch['level'], patch['body'], patch['subdir'])
                ss.patch_info = (patch['author'], patch['comment'])
            else:
                ss.patch = None
                ss.patch_info = (None, None)

While the test sets it like this

        self.patchSource = FakeSource()
        self.patchSource.repository = "repoB"
        self.patchSource.codebase = "B"
        self.patchSource.changes = []
        self.patchSource.revision = "67890"
        self.patchSource.patch_info = ("jeff", "jeff's new feature")

In this case self.patch is default set to None but this isn't the case in the actual code that is being used.

Contributor

emoon commented Feb 13, 2015

So it seems that the unit-tests fails which this change:

The coded in build_request.py looks like this when setting up the patch data:

            if ssdata['patch']:
                patch = ssdata['patch']
                ss.patch = (patch['level'], patch['body'], patch['subdir'])
                ss.patch_info = (patch['author'], patch['comment'])
            else:
                ss.patch = None
                ss.patch_info = (None, None)

While the test sets it like this

        self.patchSource = FakeSource()
        self.patchSource.repository = "repoB"
        self.patchSource.codebase = "B"
        self.patchSource.changes = []
        self.patchSource.revision = "67890"
        self.patchSource.patch_info = ("jeff", "jeff's new feature")

In this case self.patch is default set to None but this isn't the case in the actual code that is being used.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Feb 15, 2015

Member

That should be fixed in the tests, too. Bonus points for a way to ensure the fake and reality match, too.

Member

djmitche commented Feb 15, 2015

That should be fixed in the tests, too. Bonus points for a way to ensure the fake and reality match, too.

@emoon

This comment has been minimized.

Show comment
Hide comment
@emoon

emoon Feb 16, 2015

Contributor

Should I update the test(s) to make sure it works with my change also?

Contributor

emoon commented Feb 16, 2015

Should I update the test(s) to make sure it works with my change also?

@sa2ajj

This comment has been minimized.

Show comment
Hide comment
@sa2ajj

sa2ajj Feb 16, 2015

Contributor

@emoon, yes, please.

Contributor

sa2ajj commented Feb 16, 2015

@emoon, yes, please.

@emoon

This comment has been minimized.

Show comment
Hide comment
@emoon

emoon Feb 17, 2015

Contributor

Alright. I will take a look at it!

Contributor

emoon commented Feb 17, 2015

Alright. I will take a look at it!

@emoon

This comment has been minimized.

Show comment
Hide comment
@emoon

emoon Apr 17, 2015

Contributor

Sorry I totally forgot about this. I will have this sorted by tomorrow.

Contributor

emoon commented Apr 17, 2015

Sorry I totally forgot about this. I will have this sorted by tomorrow.

emoon added some commits Apr 18, 2015

Removes incorrect check of author in blamelist
This is because the author isn't added to the blamelist if patch isn't set
@emoon

This comment has been minimized.

Show comment
Hide comment
@emoon

emoon Apr 18, 2015

Contributor

I made a fix for the test. It would be good to add more elaborate testing in the future at least this PR fixes the issue that mails can't be sent to the user that caused the problem.

Contributor

emoon commented Apr 18, 2015

I made a fix for the test. It would be good to add more elaborate testing in the future at least this PR fixes the issue that mails can't be sent to the user that caused the problem.

@sa2ajj sa2ajj removed the needs work label Apr 18, 2015

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp May 3, 2015

Member

I found this in my MailNotifier branch, and fixed it the same.

Member

tardyp commented May 3, 2015

I found this in my MailNotifier branch, and fixed it the same.

@tardyp tardyp added merge me and removed please review labels May 3, 2015

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp May 3, 2015

Member

let's merge it until I get review

Member

tardyp commented May 3, 2015

let's merge it until I get review

tardyp added a commit that referenced this pull request May 3, 2015

Merge pull request #1542 from emoon/master
Changed check if patch info should be in blamelist

@tardyp tardyp merged commit 67c4055 into buildbot:master May 3, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.38%) to 83.61%
Details
@emoon

This comment has been minimized.

Show comment
Hide comment
@emoon

emoon May 4, 2015

Contributor

Thanks!

Contributor

emoon commented May 4, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment