Closed Bug 747180 Opened 10 years ago Closed 10 years ago

remove --testDate and --useID from talos perfconfigurator

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: jmuel11)

Details

(Whiteboard: [good first bug][mentor=BYK][lang=py])

Attachments

(2 files, 2 obsolete files)

in talos, we don't seem to be using the testDate variable.  This is configured in PerfConfigurator as --testDate and --useID.  There are various calls and functions to support this functionality.
Whiteboard: [mentor=BYK] → [good first bug][mentor=BYK][lang=py]
I can take a look at this.
Assignee: nobody → jmuel11
Attached patch First patch attempt (obsolete) — Splinter Review
Attachment #618836 - Flags: review?(jhammel)
Comment on attachment 618836 [details] [diff] [review]
First patch attempt

Review of attachment 618836 [details] [diff] [review]:
-----------------------------------------------------------------

Other than this minor question, LGTM.

::: talos/PerfConfigurator.py
@@ +82,1 @@
>      def _getTimeFromBuildId(self):

Did you forget to remove this one or it is used elsewhere?
Attachment #618836 - Flags: feedback+
That's related to --id, not --testDate or --useID. I don't think it's ever used but it's not related to this bug as far as I can tell.
Looking at it, though, --id seems like it was tied into the others and could be removed.
(In reply to James Mueller from comment #4)
> That's related to --id, not --testDate or --useID. I don't think it's ever
> used but it's not related to this bug as far as I can tell.

You have removed self._getTimeFromBuildId() from 181st line(the old version) that's why I mentioned this ;)
That's because it references useId. If the consensus is that _getTimeFromBuildId needs to be removed then it's easy enough to do.
++ for removing the _getTimeFromBuildId function.
Comment on attachment 618836 [details] [diff] [review]
First patch attempt

Overall good, but as recommended please get rid of _getTimeFromBuildId.  _getTime can also go as it is only used by _getTimeFromTimeStamp and _getTimeFromBuildId

Going to r- this for now, but its mostly there! :) Thanks.
Attachment #618836 - Flags: review?(jhammel) → review-
There's probably also cleanup in run_test, but we can take that in a separate bug
Hmmm, it also looks like buildid (-i, --id) isn't used at all after these are removed.  Furthermore if you use it now, you'll get an error since it is store_true:

http://hg.mozilla.org/build/talos/file/3d1065e8e7c6/talos/PerfConfigurator.py#l371

(It shouldn't be)
Do you want me to remove --id as well?
Attachment #618836 - Attachment is obsolete: true
Attachment #619169 - Flags: review?(jhammel)
Comment on attachment 619169 [details] [diff] [review]
Second patch attempt

Nice!  There's nothing I like more than seeing lots of deleted code! :)
Attachment #619169 - Flags: review?(jhammel) → review+
Will push to try to ensure all is well, and then this should be ready to land
Whiteboard: [good first bug][mentor=BYK][lang=py] → [good first bug][mentor=BYK][lang=py][talos-checkin-needed]
Attached patch unbitrot (obsolete) — Splinter Review
I bitrotted attachment 619169 [details] [diff] [review] with my last commit. unbitrotting
Oops, we missed one :(

Traceback (most recent call last):
  File "PerfConfigurator.py", line 615, in <module>
    sys.exit(main())
  File "PerfConfigurator.py", line 600, in main
    configurator.writeConfigFile()
  File "PerfConfigurator.py", line 189, in writeConfigFile
    newline = self.convertLine(line)
  File "PerfConfigurator.py", line 121, in convertLine
    newline = '%s: %s\n' % (item, getattr(self, item))
AttributeError: 'PerfConfigurator' object has no attribute 'buildid'
tested locally a bit, will repush to try
Attachment #619183 - Attachment is obsolete: true
Try run for 70bed557d894 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=70bed557d894
Results (out of 41 total builds):
    exception: 12
    success: 1
    failure: 28
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-70bed557d894
Try run for 49decdc3604d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=49decdc3604d
Results (out of 83 total builds):
    exception: 1
    success: 79
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-49decdc3604d
all these exceptions/failures passed fine on a retrigger.  I assume we are ready to push, but I will let :jhammel indicate that.
Looks good to me!
pushed: http://hg.mozilla.org/build/talos/rev/c702ff8892be

Thanks, James!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=BYK][lang=py][talos-checkin-needed] → [good first bug][mentor=BYK][lang=py]
You need to log in before you can comment on or make changes to this bug.