Misc improvements mostly Jenkins by danghai · Pull Request #113 · cki-project/sktm

danghai · 2018-07-14T08:06:55Z

I make PR to supersedes #52

coveralls · 2018-07-14T18:12:42Z

Pull Request Test Coverage Report for Build 380

31 of 64 (48.44%) changed or added relevant lines in 6 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 29.082%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sktm/patchwork.py	1	2	50.0%
sktm/executable.py	0	3	0.0%
sktm/init.py	4	13	30.77%
sktm/jenkins.py	17	37	45.95%

Files with Coverage Reduction	New Missed Lines	%
sktm/init.py	1	25.69%
sktm/jenkins.py	4	12.79%

Totals
Change from base Build 377:	0.02%
Covered Lines:	320
Relevant Lines:	928

💛 - Coveralls

danghai · 2018-07-14T19:35:23Z

@spbnick It is good to review now!

spbnick

Looks good, I have only a few minor requests. Thank you, Hai!

spbnick · 2018-07-16T09:11:45Z

                                                                    patch_url))

-                    if bres != sktm.tresult.BASELINE_FAILURE:
+                    if bres != sktm.misc.BASELINE_FAILURE:


These three above are missing TestResult in the name and wouldn't work. Please fix.

spbnick · 2018-07-16T09:22:08Z

-        self.jk = sktm.jenkins.skt_jenkins(jenkinsurl, jenkinslogin,
-                                           jenkinspassword)
-        # Jenkins project name
-        self.jobname = jenkinsjobname


A comment on the commit message: Could you update the message to say sktm.jenkins.JenkinsProject instead of sktm.jenkins.Project? Thank you.

spbnick · 2018-07-16T09:22:53Z

                self.db.set_patchset_pending(cpw.baseurl, cpw.project_id,
                                             series.get_patch_info_list())
                # Submit and remember a Jenkins build for the series
+                url_list = series.get_patch_url_list()


Same here, could you please use sktm.jenkins.JenkinsProject in the commit message?

spbnick · 2018-07-16T09:25:16Z


    def is_build_complete(self, buildid):
+        """
+        Check if a project build is complete.


Can you remove "project" from here? This class is all about a project, so no need to mention it here.

danghai · 2018-07-16T15:51:00Z

@spbnick I have updated! Let me know if you need anything change.

spbnick · 2018-07-16T16:16:31Z

@veruu, could you please review this again? @danghai rebased and cleaned it up. I think it would be great to get this in.

veruu

Thanks Hai! I noticed two things you missed from the original pull (one is a docstring so not that important, but the other one would break the functionality). Can you please resolve them and rebase?

@spbnick , your review still says "changes requested". Is this still your status? In case not, can you change it to the right one?

veruu · 2018-07-17T13:27:18Z

+            buildid:    Jenkins build ID.
+
+        Return:
+            The URL of the build result.


I can't comment on a specific commit, but the Describe sktm.jenkins.get_result_url functions commit is only adding docstring to this function, while in the original commit it's also documenting get_result one (hence why there is an asterisk in the original commit message, which is also missing in this one).

veruu · 2018-07-17T13:36:01Z

                                    subject=series.subject,
                                    emails=series.email_addr_set,
-                                    patchwork=series.get_patch_url_list(),
+                                    patchwork=url_list,


The method parameter is still named patchwork here (while it's changed in the build(), so this commit would break things)

spbnick · 2018-07-17T13:51:22Z

Thank you, @veruu for spotting the omissions! Approving and handing over to you now :)

spbnick · 2018-07-18T17:22:25Z

@danghai, do you think you could rebase this again and address @veruu's comments?

danghai · 2018-07-18T18:02:52Z

@spbnick Sure. I will do it when I have a chance

Move sktm.tresult to sktm.misc.TestResult to fix composition, so that modules lower in the hierarchy (e.g. sktm.jenkins) do not have to import the module above (i.e. sktm).

Instead of creating and using a "Jenkins Interface" (sktm.jenkins.JenkinsProject), and then supplying a project name ("jobname") with every method call, create and use a "Jenkins Project Interface" (sktm.jenkins.JenkinsProject) and only supply the project name on its creation. This simplifes the interface, and removes a bit of code. This also prepares for abstracting Jenkins away.

Spell out "patch URL list" instead of writing "patchwork" in sktm.jenkins.JenkinsProject, to make clear what's being accepted/returned. Leave updating the Jenkins project parameter name for later.

Create a complete Jenkins project interface instance and pass it to the watcher, instead of having the watcher create it. This prepares the watcher to using abstract scheduler interface, instead of Jenkins specifically.

danghai · 2018-07-19T09:34:20Z

@spbnick @veruu I rebased and addressed comments. It is good to review now :)

veruu

Thanks Hai! The pull looks good now!

spbnick · 2018-07-19T11:13:05Z

Thank you very much, @veruu and @danghai 😄!

danghai mentioned this pull request Jul 14, 2018

Misc improvements mostly Jenkins #52

Closed

danghai force-pushed the sktm_misc branch 2 times, most recently from f364825 to 9ad0a99 Compare July 14, 2018 18:09

danghai force-pushed the sktm_misc branch from 9ad0a99 to 00bc634 Compare July 14, 2018 18:18

spbnick suggested changes Jul 16, 2018

View reviewed changes

danghai force-pushed the sktm_misc branch 2 times, most recently from 7d22fcb to 75c8d72 Compare July 16, 2018 15:48

spbnick requested a review from veruu July 16, 2018 16:15

veruu suggested changes Jul 17, 2018

View reviewed changes

spbnick previously approved these changes Jul 17, 2018

View reviewed changes

danghai dismissed spbnick’s stale review via 3d58e0f July 19, 2018 09:17

danghai force-pushed the sktm_misc branch from 75c8d72 to 3d58e0f Compare July 19, 2018 09:17

danghai added 7 commits July 19, 2018 02:29

Rename class tresult to TestResult, and move into sktm.misc

37e2ce2

Move sktm.tresult to sktm.misc.TestResult to fix composition, so that modules lower in the hierarchy (e.g. sktm.jenkins) do not have to import the module above (i.e. sktm).

Spell out patch URL list in sktm.jenkins

94adc08

Spell out "patch URL list" instead of writing "patchwork" in sktm.jenkins.JenkinsProject, to make clear what's being accepted/returned. Leave updating the Jenkins project parameter name for later.

Specify patch order for patch_url_list args

6a92aba

Document sktm.jenkins.Project.is_build_complete()

0e55b3e

Create Jenkins project interface outside watcher

d9b6c0e

Create a complete Jenkins project interface instance and pass it to the watcher, instead of having the watcher create it. This prepares the watcher to using abstract scheduler interface, instead of Jenkins specifically.

Describe sktm.jenkins.get_result* functions

390543a

danghai force-pushed the sktm_misc branch from 3d58e0f to 390543a Compare July 19, 2018 09:31

veruu approved these changes Jul 19, 2018

View reviewed changes

spbnick merged commit 8e6f329 into cki-project:master Jul 19, 2018

danghai deleted the sktm_misc branch July 19, 2018 16:57

Conversation

danghai commented Jul 14, 2018

Uh oh!

coveralls commented Jul 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 380

💛 - Coveralls

Uh oh!

danghai commented Jul 14, 2018

Uh oh!

spbnick left a comment

Choose a reason for hiding this comment

Uh oh!

spbnick Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

spbnick Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

spbnick Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

spbnick Jul 16, 2018

Choose a reason for hiding this comment

Uh oh!

danghai commented Jul 16, 2018

Uh oh!

spbnick commented Jul 16, 2018

Uh oh!

veruu left a comment

Choose a reason for hiding this comment

Uh oh!

veruu Jul 17, 2018

Choose a reason for hiding this comment

Uh oh!

veruu Jul 17, 2018

Choose a reason for hiding this comment

Uh oh!

spbnick commented Jul 17, 2018

Uh oh!

spbnick commented Jul 18, 2018

Uh oh!

danghai commented Jul 18, 2018

Uh oh!

danghai commented Jul 19, 2018

Uh oh!

veruu left a comment

Choose a reason for hiding this comment

Uh oh!

spbnick commented Jul 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jul 14, 2018 •

edited

Loading