-
Notifications
You must be signed in to change notification settings - Fork 18
Flip all jobs for mozilla-central to Marionette (#576) #581
Conversation
manifests = [firefox_ui_tests.manifest_remote] | ||
command.extend(manifests) | ||
|
||
print 'Execute tests: %s' % command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this message useful for the resulting log messages. It seems so, but maybe more context around it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What type of context do you think of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could it be 'executing firefox-ui-tests %s command: %s" % (options.type,
command). The more information we have to quick look at a failure in a
random bug, the easier it is for a random person to understand. Not
required, but probably good to have.
On Tue, May 19, 2015 at 4:58 PM, Henrik Skupin notifications@github.com
wrote:
In jenkins-master/jobs/scripts/workspace/runtests.py
#581 (comment):
if options.update_channel:
command.append('--update-channel=%s' % options.update_channel)
if options.update_target_version:
command.append('--update-target-version=%s' % options.update_target_version)
if options.update_target_build_id:
command.append('--update-target-buildid=%s' % options.update_target_build_id)
elif options.type == 'functional':
manifests = [firefox_puppeteer.manifest, firefox_ui_tests.manifest_functional]
command.extend(manifests)
elif options.type == 'remote':
manifests = [firefox_ui_tests.manifest_remote]
command.extend(manifests)
print 'Execute tests: %s' % command
What type of context do you think of?
—
Reply to this email directly or view it on GitHub
https://github.com/mozilla/mozmill-ci/pull/581/files#r30646061.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ´firefox-ui-tests´ command is only used for functional and remote tests. For update tests there is a ´firefox-ui-update´ command. Please be aware that exactly this command is already listed in the output.
Looks like I cannot comment out the lines in the config files. I will remove them instead. |
import firefox_puppeteer | ||
|
||
command = [ | ||
'firefox-ui-update' if options.type == 'update' else 'firefox-ui-tests', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this style of "value IF ELSE other_value" used throughout firefox-ui-tests/mozmill? We don't use it in our other tests, though I would try to keep it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said it looks ugly. I could create separate classes for them in that file. But when you wrote this I was thinking more about all that code and wonder if we are better to kill it all. We could make the run_tests.py script a simply activation script for the environment and let it execute a passed in command and all its arguments. That would make it way simpler, and we wouldn't need any other abstraction around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this is actually not working. Problem here are the different path delimiters for the path of builds and manifests between Windows and OS X/Linux. I was suggesting to have different entry script for functional and remote via bug 1164637, where default manifests would have been set internally. But @chmanchester was against this addition so we dropped that. Looks like we have to go this way now with having more code in this file. :( Otherwise lets convince @chmanchester so we can get those entry scripts for real.
two comments, otherwise, this seems good enough to rubber stamp. |
r+ |
Thanks David. I will merge this PR into staging now and make it immediately active in that instance. |
d5df229
to
377d2b6
Compare
With this PR we fix issue #576 by flipping all jobs for mozilla-central to Marionette. Testruns we do not use anymore, which include endurance tests and addon tests, I have removed. So each job is doing the following:
I have also removed unnecessary job parameters including the report url. Once we know how to work with treeherder we can add it back. But so long I don't see a reason to keep it.
The runtests.py does not contain a great class hierarchy given that we might not need it that long before we can run our tests on buildbot. If you think its worth implementing classes for each type of Runner I can happily do that.
I have updated the content of the failure email. It now also contains the changeset AND the list of test failures. So you can directly see what was going on without having to go to the Jenkins site. Further we have HTML reports including screenshots. Investigating problems will become much easier! Hurray!
Currently we cannot enable the junit reporting for update tests, because we are writing invalid log files due to the multiple invocation of Marionette for those tests. Once this has been fixed we can re-enable the feature. Until then its not that important for us.