747180 - remove --testDate and --useID from talos perfconfigurator

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 1

[James Mueller]

I can take a look at this.

Comment 2

[James Mueller]

Attachment: First patch attempt

Comment 3

[Burak Yiğit Kaya [:BYK]]

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?

Comment 4

[James Mueller]

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.

Comment 5

[James Mueller]

Looking at it, though, --id seems like it was tied into the others and could be removed.

Comment 6

[Burak Yiğit Kaya [:BYK]]

(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 ;)

Comment 7

[James Mueller]

That's because it references useId. If the consensus is that _getTimeFromBuildId needs to be removed then it's easy enough to do.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

++ for removing the _getTimeFromBuildId function.

Comment 9

[Jeff Hammel]

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.

Comment 10

[Jeff Hammel]

There's probably also cleanup in run_test, but we can take that in a separate bug

Comment 11

[Jeff Hammel]

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)

Comment 12

[James Mueller]

Do you want me to remove --id as well?

Comment 13

[James Mueller]

Attachment: Second patch attempt

Comment 14

[Jeff Hammel]

Comment on attachment 619169 [details] [diff] [review]
Second patch attempt

Nice!  There's nothing I like more than seeing lots of deleted code! :)

Comment 15

[Jeff Hammel]

Will push to try to ensure all is well, and then this should be ready to land

Comment 16

[Jeff Hammel]

Attachment: unbitrot

I bitrotted attachment 619169 [details] [diff] [review] with my last commit. unbitrotting

Comment 17

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=70bed557d894

Comment 18

[Jeff Hammel]

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'

Comment 19

[Jeff Hammel]

Attachment: unbitrot plus remove from replacements

tested locally a bit, will repush to try

Comment 20

[Jeff Hammel]

new try push is here: https://tbpl.mozilla.org/?tree=Try&rev=49decdc3604d

Comment 21

[Mozilla RelEng Bot]

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/[e-mail]-70bed557d894

Comment 22

[Mozilla RelEng Bot]

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/[e-mail]-49decdc3604d

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

all these exceptions/failures passed fine on a retrigger.  I assume we are ready to push, but I will let :jhammel indicate that.

Comment 24

[Jeff Hammel]

Looks good to me!

Comment 25

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/c702ff8892be

Thanks, James!