Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130833003:
Telemetry ChromeOS Enterprise enrollment (Closed)
|
Created:
5 years, 7 months ago by David Abrahams Modified:
5 years, 6 months ago Reviewers:
achuithb CC:
chromium-reviews, telemetry-reviews_chromium.org, crosg3_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTelemetry ChromeOS Enterprise enrollment
The enterprise enrollment oobe test helper needed some additional work
that I discovered once I started writing tests. This has been verified
to work on a VM with the new email/password separated UI.
BUG=474707
TESTING=Wrote a small autotest that passes in VM
Committed: https://crrev.com/1c5e795e27d7ac359ff30144e513914de3483552
Cr-Commit-Position: refs/heads/master@{#330035}
Patch Set 1 #Patch Set 2 : Removing some unrelated changes #Patch Set 3 : Cleanup #
Total comments: 1
Patch Set 4 : Reuse _NavigateWebViewLogin and correct comment #
Total comments: 2
Patch Set 5 : Adding crbug/486904 #Messages
Total messages: 21 (6 generated)
resetswitch@chromium.org changed reviewers: + achuith@chromium.org
https://codereview.chromium.org/1130833003/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/oobe.py (right): https://codereview.chromium.org/1130833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/oobe.py:63: self._NavigateWebViewEntry('identifierId', username) I guess line 104 is the only line not relevant for the enterprise enrollment flow in _NavigateLogin? It would be nice to use _NavigateLogin and use a boolean to skip line 104 for enterprise enrollment?
On 2015/05/08 05:48:58, achuithb wrote: > https://codereview.chromium.org/1130833003/diff/40001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/backends/chrome/oobe.py (right): > > https://codereview.chromium.org/1130833003/diff/40001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/backends/chrome/oobe.py:63: > self._NavigateWebViewEntry('identifierId', username) > I guess line 104 is the only line not relevant for the enterprise enrollment > flow in _NavigateLogin? > > It would be nice to use _NavigateLogin and use a boolean to skip line 104 for > enterprise enrollment? Good suggestion, done.
Rest of this looks ok! https://codereview.chromium.org/1130833003/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/oobe.py (right): https://codereview.chromium.org/1130833003/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/oobe.py:68: self.WaitForJavaScriptExpression(""" We need something here along the lines of: self.WaitForJavaScriptExpression('Oobe.enrollmentSucceededForTesting()'); We cannot have telemetry code depend on the specifics of the UI in the oobe html. I'm ok to lg2m this change if you want to open a P1 bug, add a TODO here, and promise to fix this in a subsequent CL soon.
On 2015/05/11 20:01:48, achuithb wrote: > Rest of this looks ok! > > https://codereview.chromium.org/1130833003/diff/60001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/backends/chrome/oobe.py (right): > > https://codereview.chromium.org/1130833003/diff/60001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/backends/chrome/oobe.py:68: > self.WaitForJavaScriptExpression(""" > We need something here along the lines of: > self.WaitForJavaScriptExpression('Oobe.enrollmentSucceededForTesting()'); > > We cannot have telemetry code depend on the specifics of the UI in the oobe > html. > > I'm ok to lg2m this change if you want to open a P1 bug, add a TODO here, and > promise to fix this in a subsequent CL soon. Done. I've opened crbug.com/486904 and I'll address it in the next week if that's ok.
https://codereview.chromium.org/1130833003/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/oobe.py (right): https://codereview.chromium.org/1130833003/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/oobe.py:63: self._NavigateWebViewEntry('identifierId', username) Did this get reverted?
On 2015/05/11 21:56:33, achuithb wrote: > https://codereview.chromium.org/1130833003/diff/80001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/backends/chrome/oobe.py (right): > > https://codereview.chromium.org/1130833003/diff/80001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/backends/chrome/oobe.py:63: > self._NavigateWebViewEntry('identifierId', username) > Did this get reverted? Whoh I lost the earlier changes in a bad merge. Thank you for noticing. Fixing it now.
Patchset #5 (id:80001) has been deleted
Added bug comment without blasting away all the other changes this time. https://codereview.chromium.org/1130833003/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/oobe.py (right): https://codereview.chromium.org/1130833003/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/oobe.py:68: self.WaitForJavaScriptExpression(""" On 2015/05/11 20:01:48, achuithb wrote: > We need something here along the lines of: > self.WaitForJavaScriptExpression('Oobe.enrollmentSucceededForTesting()'); > > We cannot have telemetry code depend on the specifics of the UI in the oobe > html. > > I'm ok to lg2m this change if you want to open a P1 bug, add a TODO here, and > promise to fix this in a subsequent CL soon. Done.
The CQ bit was checked by resetswitch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130833003/100001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2015/05/12 00:26:40, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Achuith, look better this time? Thanks for your help.
lgtm. Sorry for the wait, busy week.
The CQ bit was checked by resetswitch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130833003/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1c5e795e27d7ac359ff30144e513914de3483552 Cr-Commit-Position: refs/heads/master@{#330035}
Message was sent while issue was closed.
Patchset #6 (id:120001) has been deleted |
