Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622303003:
Allows a user to create_health_reports. (Closed)
DescriptionAllows a user to create_health_reports.
Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort.
If you want to put something in the datastore, feel free to try so here:
https://dev-jessimb-156d874e-dot-chromeperf.appspot.com/create_health_report
BUG=catapult:#3141
Review-Url: https://codereview.chromium.org/2622303003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/af5154d43a513b9977fef08e3c79dddf8f9c6567
Patch Set 1 #Patch Set 2 : removing changes from other CL #
Total comments: 20
Patch Set 3 : Updated to polymer elements, validation of bots, other things related to code review comments. #Patch Set 4 : Updated to polymer elements, validation of bots, other things related to code review comments. #
Total comments: 20
Patch Set 5 : added testing #
Total comments: 23
Patch Set 6 : response to comments #Patch Set 7 : added check that table name was unique #
Total comments: 1
Patch Set 8 : updated a test #
Messages
Total messages: 30 (13 generated)
Description was changed from ========== Upload a table config ========== to ========== Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. ==========
jessimb@chromium.org changed reviewers: + eakuefner@chromium.org
PTAL
Description was changed from ========== Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. ========== to ========== Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. BUG=catapult:#3141 ==========
eakuefner@chromium.org changed reviewers: + sullivan@chromium.org
lgtm, but I guess I'm still wondering about the whole Jinja thing. Again, eliminating it is probably out of scope for this CL, but I'd like Annie to confirm. Also, please: 1. Make the title the first line of your description, and put an extra newline after it (see https://codereview.chromium.org/2616153002 for an example) 2. Set BUG=
Description was changed from ========== Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. BUG=catapult:#3141 ========== to ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. BUG=catapult:#3141 ==========
On 2017/01/11 20:18:49, eakuefner wrote: > lgtm, but I guess I'm still wondering about the whole Jinja thing. Again, > eliminating it is probably out of scope for this CL, but I'd like Annie to > confirm. I think this should be a polymer element from the start.
https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml File dashboard/app.yaml (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml#newc... dashboard/app.yaml:116: secure: always Why do we restrict report creation to admins? Shouldn't it be something that all Google users can do? https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:23: master_bot = self.request.get('masterBot').splitlines() It's poor security to rely on the user to supply whether or not this should be internal only. We should either: 1 Make the report internal_only if any of the bots are internal_only 2 Make each report public but not display data from internal_only bots to external users. This would be secure because our hooks prevent us from displaying this data accidentally. I think 1) is better because it's less error-prone; 2 should work but it's too complicated. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:32: {'msg': 'Health Report Created. View here: [TODO jessimb]'}) We're trying to get away from jinja templates, because it's really hard to have both jinja templates and polymer templates together. We want to get to a point where we only have polymer templates. This endpoint should be called from the client with simple_xhr and return JSON that is used to populate a polymer template. The JSON should probably return the URL, or an error message, and the polymer template should update based on that. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/nav-bar.html (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/nav-bar.html:176: <li><a href="/create_health_report" target="_blank">Create Health Report</a></li> Again, unclear why this is an admin endpoint. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:15: Example test_layout: [TODO @jessimb] These examples sshould be filled in. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:31: bots = ndb.StringProperty(repeated=True) Shouldn't this be ndb.KeyProperty(repeated=True) for the keys of the bots? If the strings don't correspond to real bots we shouldn't store them. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:37: table_layout = ndb.TextProperty() This needs to be better documented.
A couple open question in the comments for sullivan@ https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml File dashboard/app.yaml (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml#newc... dashboard/app.yaml:116: secure: always On 2017/01/11 at 20:59:15, sullivan wrote: > Why do we restrict report creation to admins? Shouldn't it be something that all Google users can do? Because report creation shouldn't need to be done but once, I thought it would make sense to keep it at a privileged level. I guess there isn't any data that people can change that isn't public. Should I just move this to entirely public? If so, I'm not sure it needs its own spot in the nav bar due to the low frequency of use. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:23: master_bot = self.request.get('masterBot').splitlines() On 2017/01/11 at 20:59:15, sullivan wrote: > It's poor security to rely on the user to supply whether or not this should be internal only. We should either: > > 1 Make the report internal_only if any of the bots are internal_only > 2 Make each report public but not display data from internal_only bots to external users. This would be secure because our hooks prevent us from displaying this data accidentally. > > I think 1) is better because it's less error-prone; 2 should work but it's too complicated. Would that check go as a datastore pre-hook? https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:32: {'msg': 'Health Report Created. View here: [TODO jessimb]'}) On 2017/01/11 at 20:59:15, sullivan wrote: > We're trying to get away from jinja templates, because it's really hard to have both jinja templates and polymer templates together. We want to get to a point where we only have polymer templates. > > This endpoint should be called from the client with simple_xhr and return JSON that is used to populate a polymer template. The JSON should probably return the URL, or an error message, and the polymer template should update based on that. Making this a polymer element. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/nav-bar.html (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/nav-bar.html:176: <li><a href="/create_health_report" target="_blank">Create Health Report</a></li> On 2017/01/11 at 20:59:15, sullivan wrote: > Again, unclear why this is an admin endpoint. (see other response) https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:31: bots = ndb.StringProperty(repeated=True) On 2017/01/11 at 20:59:15, sullivan wrote: > Shouldn't this be ndb.KeyProperty(repeated=True) for the keys of the bots? If the strings don't correspond to real bots we shouldn't store them. Should there be a check in here (well, in the POST) to verify they are real bots?
https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml File dashboard/app.yaml (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml#newc... dashboard/app.yaml:116: secure: always On 2017/01/11 21:31:21, jessimb wrote: > On 2017/01/11 at 20:59:15, sullivan wrote: > > Why do we restrict report creation to admins? Shouldn't it be something that > all Google users can do? > > Because report creation shouldn't need to be done but once, I thought it would > make sense to keep it at a privileged level. I guess there isn't any data that > people can change that isn't public. > Should I just move this to entirely public? If so, I'm not sure it needs its own > spot in the nav bar due to the low frequency of use. We should probably restrict to internal users to keep spam low, and record the username of the person who created the report. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:23: master_bot = self.request.get('masterBot').splitlines() On 2017/01/11 21:31:21, jessimb wrote: > On 2017/01/11 at 20:59:15, sullivan wrote: > > It's poor security to rely on the user to supply whether or not this should be > internal only. We should either: > > > > 1 Make the report internal_only if any of the bots are internal_only > > 2 Make each report public but not display data from internal_only bots to > external users. This would be secure because our hooks prevent us from > displaying this data accidentally. > > > > I think 1) is better because it's less error-prone; 2 should work but it's too > complicated. > > Would that check go as a datastore pre-hook? yep! https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:31: bots = ndb.StringProperty(repeated=True) On 2017/01/11 21:31:21, jessimb wrote: > On 2017/01/11 at 20:59:15, sullivan wrote: > > Shouldn't this be ndb.KeyProperty(repeated=True) for the keys of the bots? If > the strings don't correspond to real bots we shouldn't store them. > > Should there be a check in here (well, in the POST) to verify they are real > bots? Yep! Could either be in the POST or you could make a CreateTableConfig() method that creates it and does the checks (or a pre hook, but that moves the error a bit far from the caller)
Description was changed from ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. BUG=catapult:#3141 ========== to ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-30d06a0d-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ==========
PTAL I factored /create_health_report into a polymer element, added checks to the data in table_config.py, and only allow internal users to submit data (but they can view the form). Be sure to try it out! https://dev-jessimb-30d06a0d-dot-chromeperf.appspot.com/create_health_report (note, the link that it gives you to view the report will be a 404 until the other CL lands) Thanks! https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml File dashboard/app.yaml (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/app.yaml#newc... dashboard/app.yaml:116: secure: always On 2017/01/11 at 21:38:03, sullivan wrote: > On 2017/01/11 21:31:21, jessimb wrote: > > On 2017/01/11 at 20:59:15, sullivan wrote: > > > Why do we restrict report creation to admins? Shouldn't it be something that > > all Google users can do? > > > > Because report creation shouldn't need to be done but once, I thought it would > > make sense to keep it at a privileged level. I guess there isn't any data that > > people can change that isn't public. > > Should I just move this to entirely public? If so, I'm not sure it needs its own > > spot in the nav bar due to the low frequency of use. > > We should probably restrict to internal users to keep spam low, and record the username of the person who created the report. Done https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:23: master_bot = self.request.get('masterBot').splitlines() On 2017/01/11 at 21:38:03, sullivan wrote: > On 2017/01/11 21:31:21, jessimb wrote: > > On 2017/01/11 at 20:59:15, sullivan wrote: > > > It's poor security to rely on the user to supply whether or not this should be > > internal only. We should either: > > > > > > 1 Make the report internal_only if any of the bots are internal_only > > > 2 Make each report public but not display data from internal_only bots to > > external users. This would be secure because our hooks prevent us from > > displaying this data accidentally. > > > > > > I think 1) is better because it's less error-prone; 2 should work but it's too > > complicated. > > > > Would that check go as a datastore pre-hook? > > yep! I have put this in the new CreateTableConfig() which also checks validity of master/bots https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:15: Example test_layout: [TODO @jessimb] On 2017/01/11 at 20:59:15, sullivan wrote: > These examples sshould be filled in. These might change. Filled in for now. https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:31: bots = ndb.StringProperty(repeated=True) On 2017/01/11 at 21:38:03, sullivan wrote: > On 2017/01/11 21:31:21, jessimb wrote: > > On 2017/01/11 at 20:59:15, sullivan wrote: > > > Shouldn't this be ndb.KeyProperty(repeated=True) for the keys of the bots? If > > the strings don't correspond to real bots we shouldn't store them. > > > > Should there be a check in here (well, in the POST) to verify they are real > > bots? > > Yep! Could either be in the POST or you could make a CreateTableConfig() method that creates it and does the checks (or a pre hook, but that moves the error a bit far from the caller) Done https://codereview.chromium.org/2622303003/diff/20001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:37: table_layout = ndb.TextProperty() On 2017/01/11 at 20:59:15, sullivan wrote: > This needs to be better documented. Comments added at the top of the file.
This needs tests! create_health_report_test.py: testPost_CreateTable: Add some bots to the datastore post() valid table parameters assert the TableConfig was created with the expected parameters assert the json response contains the name of the table config testPost_InvalidBot: post() with garbage bot parameters assert no TableConfigs were created assert the response contains an appropriate error message testPost_InvalidTableLayoutJSON: add a bot to the data store post with valid() bot/test_path parameters and incorrect table_layout JSON assert the response contains an appropriate error message (you can use test-driven development to learn about how JSONProperty works!) create-health-report-page-test.html: A basic instantiation test to ensure there are no serious errors on the page and make it easy to tweak HTML/CSS is all you need. Bonus points for: * testing input validation * having a test that calls element.onSubmit() and mocks simple_xhr to check that the right parameters were sent. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:22: def post(self): Should be XSRF protected. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:31: {'error': 'Unauthorized access, please use corp account to login.'})) chromium account https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:40: layout=table_layout, username=user.email()) This code should check if the TableConfig was created and either: 1. Write out the name 2. Return {'error': <error string from table creation method>} https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page.html (right): https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:67: <b>User-friendly name:</b><br> You should probably add some basic input validation for these: https://elements.polymer-project.org/elements/paper-input?active=paper-input-... https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:68: <paper-input id="name" label="Report Name"></paper-input> The inputs should use two-way binding to a property: https://www.polymer-project.org/1.0/docs/devguide/data-binding#two-way-bindings https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:87: Enter a serialized dict to specify the layout. For example: <br> Should probably say a JSON dict? https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:131: 'tableTests': this.$.tests.value, These are the properties that should have two-way bindings to the input elements. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:49: table_layout = ndb.TextProperty() Thanks for the documentation! Looks like this should be ndb.JsonProperty https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:69: internal_only = True I think this should probably return a dict or a tuple: (TableConfig, message). If any of the bots are not valid, it should return something like: (None, 'Invalid bot ChromiumPref/nexus-4') https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:72: internal_only=internal_only, username=username).put() Please return the created TableConfig.
I have added much tests. Let me know if I am missing any or if they are done incorrectly. I don't think I get many bonus points :( Testing input validation - I added required and the server checks for proper layout and bots. Is that all? Side note, paper-textarea wants to validate on page load, so 'input required' appears when you visit the end point. After way too long trying to fix it, I've determined we will just be very up front about telling users that the input is required on the 3 text areas. >>having a test that calls element.onSubmit() and mocks simple_xhr to check that the right parameters were sent. I have no idea how to do that. testing_common.addXhrMock() requires a response, and none of the other functions in testin_common.html seemed to provide much help. Let me know what you think about the token validation in create_health_report.py; I essentially took the functionality that @xsrf.TokenRequired uses and called it when I needed it. I'm not 100% sure I'm using validating the table_layout correctly. ndb.JsonProperty() doesn't seem to care what you give it. The link in the description is an updated demo. Try entering in some bad data. Hopefully this CL can land soon! https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:22: def post(self): On 2017/01/12 at 14:14:29, sullivan wrote: > Should be XSRF protected. Done https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:31: {'error': 'Unauthorized access, please use corp account to login.'})) On 2017/01/12 at 14:14:29, sullivan wrote: > chromium account Right. Done. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report.py:40: layout=table_layout, username=user.email()) On 2017/01/12 at 14:14:29, sullivan wrote: > This code should check if the TableConfig was created and either: > 1. Write out the name > 2. Return {'error': <error string from table creation method>} Done. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page.html (right): https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:67: <b>User-friendly name:</b><br> On 2017/01/12 at 14:14:29, sullivan wrote: > You should probably add some basic input validation for these: https://elements.polymer-project.org/elements/paper-input?active=paper-input-... I added 'required'...server handles bots and layout validation. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:68: <paper-input id="name" label="Report Name"></paper-input> On 2017/01/12 at 14:14:29, sullivan wrote: > The inputs should use two-way binding to a property: https://www.polymer-project.org/1.0/docs/devguide/data-binding#two-way-bindings Done. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:87: Enter a serialized dict to specify the layout. For example: <br> On 2017/01/12 at 14:14:29, sullivan wrote: > Should probably say a JSON dict? Done. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:131: 'tableTests': this.$.tests.value, On 2017/01/12 at 14:14:29, sullivan wrote: > These are the properties that should have two-way bindings to the input elements. Done. https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:49: table_layout = ndb.TextProperty() On 2017/01/12 at 14:14:29, sullivan wrote: > Thanks for the documentation! Looks like this should be ndb.JsonProperty Okay! https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:69: internal_only = True On 2017/01/12 at 14:14:29, sullivan wrote: > I think this should probably return a dict or a tuple: > (TableConfig, message). > > If any of the bots are not valid, it should return something like: > (None, 'Invalid bot ChromiumPref/nexus-4') Sgtm https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:72: internal_only=internal_only, username=username).put() On 2017/01/12 at 14:14:29, sullivan wrote: > Please return the created TableConfig. Yeah, wasn't really sure what to do here.
Description was changed from ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-30d06a0d-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ========== to ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-00bab161-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ==========
> https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... > dashboard/dashboard/create_health_report.py:31: {'error': 'Unauthorized access, please use corp account to login.'})) > On 2017/01/12 at 14:14:29, sullivan wrote: > > chromium account > > Right. Done. > Also - I took out the isInternalUser logic because I couldn't get my tests to think the user was internal. Do we want to check this?
On 2017/01/13 04:05:15, jessimb wrote: > > > https://codereview.chromium.org/2622303003/diff/60001/dashboard/dashboard/cre... > > dashboard/dashboard/create_health_report.py:31: {'error': 'Unauthorized > access, please use corp account to login.'})) > > On 2017/01/12 at 14:14:29, sullivan wrote: > > > chromium account > > > > Right. Done. > > > > Also - I took out the isInternalUser logic because I couldn't get my tests to > think the user was internal. Do we want to check this? Ideally we would. It does look like your tests set up the mocks correctly for an internal user. The easiest way to get help is to post a patchset with the failing tests and debugging info, run it through the trybots, and link a failure so we can all look at it.
On 2017/01/13 04:02:30, jessimb wrote: > I have added much tests. Let me know if I am missing any or if they are done > incorrectly. I added some comments: The create_health_report tests should always post to that endpoint. The UI tests should mock out the XHRs for both calls and call onSubmit() > I don't think I get many bonus points :( > Testing input validation - I added required and the server checks for proper > layout and bots. Is that all? Side note, paper-textarea wants to validate on > page load, so 'input required' appears when you visit the end point. After way > too long trying to fix it, I've determined we will just be very up front about > telling users that the input is required on the 3 text areas. Yeah, this type of input validation is the way it's done in material design, so it's fine. > >>having a test that calls element.onSubmit() and mocks simple_xhr to check that > the right parameters were sent. > I have no idea how to do that. testing_common.addXhrMock() requires a response, > and none of the other functions in testin_common.html seemed to provide much > help. Ah, that's right, it doesn't check its callers like a true mock. I think the easiest thing to do is give it a member variable requestsMade which is an array you append requestStr to each time send() is called: https://github.com/catapult-project/catapult/blob/master/dashboard/dashboard/... Then you can do assertions on the requests that were made and their order. > Let me know what you think about the token validation in > create_health_report.py; I essentially took the functionality that > @xsrf.TokenRequired uses and called it when I needed it. Looks good! > I'm not 100% sure I'm using validating the table_layout correctly. > ndb.JsonProperty() doesn't seem to care what you give it. It's definitely better than nothing! I think a TODO for when we better understand what we want the JSON to be is okay for now. > The link in the description is an updated demo. Try entering in some bad data. > Hopefully this CL can land soon! Getting a lot closer! https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:36: testing_common.SetIsInternalUser('internal@chromium.com', True) This wasn't working? https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:79: self.SetCurrentUser('internal@chromium.org', is_admin=True) This should be handled in tear down https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:87: self.assertIn('my_sample_config', response) You should also do an ndb.Key('TableConfig', 'my_sample_config') and check that it has the expected data. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:93: self.assertIn('Please fill out the form entirely.', response) Also assert that a query of the datastore for TableConfig returns 0 entities (this didn't add anything to the datastore) https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:104: self.assertIn('Invalid Master/Bot: garbage/moarGarbage', response) Same thing as above about asserting no TableConfigs added to datastore. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:115: self.assertTrue(table.internal_only) This should probably go in a table_config_test.py file. There should be a test here that does a post to /create_health_report with this table config, asserts that the response has the correct name, and asserts that the table is in the datastore. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:164: self.assertIn('Invalid JSON for table layout', msg) Same comment as above on all these tests: tests that directly call table_config should be in table_config_test.py. These are good tests, but they should be doing a post() to /create_health_report and checking the response and datastore. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page-test.html (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page-test.html:27: testing_common.addXhrMock('*', JSON.stringify(mockResponse)); Adding a mock for '*' here won't work because you'll want multiple responses: 1) The token 2) The create So you need to call addXhrMock() and then add() on the response to generate the two responses. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page-test.html:29: this.addHTMLOutput(page); After that you should be able to call page.onSubmit() https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page.html (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:62: <div class="error">{{error}}</div> I'm on my laptop and the entire page does not fit on the screen. This error message is very hard to find when I am clicking the submit button. I think it should go directly above the submit button. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:102: username=username) Don't you need to set the id of the table_config in order to look it up by key later? https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:106: ### PROBABLY SHOULD VERIFY JSON OF TABLE LAYOUT I would move this comment to line 96, and it should be in the correct style: # TODO(jessimb): verify JSON of table layout.
PTAL. I fixed the issues in the comments (in particular cleaned up the python testing file). I also added more the html test page. As is, the html test will set the values of the fields, send the request, and the page is updated with my supplied response. The values I send via tableParams must match the page variables in order for the request to work - is that sufficient to test for request accuracy? Order matters here because testing_common.paramString() does a sort(). Thanks so much for bearing with me on all of this! https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:36: testing_common.SetIsInternalUser('internal@chromium.com', True) On 2017/01/13 at 14:44:13, sullivan wrote: > This wasn't working? Wow. chromium.com. Works now... https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:79: self.SetCurrentUser('internal@chromium.org', is_admin=True) On 2017/01/13 at 14:44:13, sullivan wrote: > This should be handled in tear down Well, it would be handled in the next setUp() I think. Regardless, it doesn't belong here. Removed. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:87: self.assertIn('my_sample_config', response) On 2017/01/13 at 14:44:13, sullivan wrote: > You should also do an ndb.Key('TableConfig', 'my_sample_config') and check that it has the expected data. Done. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:93: self.assertIn('Please fill out the form entirely.', response) On 2017/01/13 at 14:44:13, sullivan wrote: > Also assert that a query of the datastore for TableConfig returns 0 entities (this didn't add anything to the datastore) For some reason I thought you could only verify what happened by checking the response from the post (hence the rest of my tests checking the call directly). Fixed. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:104: self.assertIn('Invalid Master/Bot: garbage/moarGarbage', response) On 2017/01/13 at 14:44:14, sullivan wrote: > Same thing as above about asserting no TableConfigs added to datastore. Done https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:115: self.assertTrue(table.internal_only) On 2017/01/13 at 14:44:13, sullivan wrote: > This should probably go in a table_config_test.py file. > > There should be a test here that does a post to /create_health_report with this table config, asserts that the response has the correct name, and asserts that the table is in the datastore. That test exists. I have updated this and the next test to do a POST and verify the internal_only of what is in the datastore. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/cre... dashboard/dashboard/create_health_report_test.py:164: self.assertIn('Invalid JSON for table layout', msg) On 2017/01/13 at 14:44:13, sullivan wrote: > Same comment as above on all these tests: tests that directly call table_config should be in table_config_test.py. These are good tests, but they should be doing a post() to /create_health_report and checking the response and datastore. I didn't realize I could query the datastore! The testing framework we have rocks. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page-test.html (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page-test.html:27: testing_common.addXhrMock('*', JSON.stringify(mockResponse)); On 2017/01/13 at 14:44:14, sullivan wrote: > Adding a mock for '*' here won't work because you'll want multiple responses: > 1) The token > 2) The create > > So you need to call addXhrMock() and then add() on the response to generate the two responses. I think I have done this. But I'm not clear about the response part. The response isn't ever returned? https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/create-health-report-page.html (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/create-health-report-page.html:62: <div class="error">{{error}}</div> On 2017/01/13 at 14:44:14, sullivan wrote: > I'm on my laptop and the entire page does not fit on the screen. This error message is very hard to find when I am clicking the submit button. I think it should go directly above the submit button. Oo good idea. I don't know why assuming the user noticed that the page moved slightly and they should scroll to the top was what I decided it should do. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/mod... File dashboard/dashboard/models/table_config.py (right): https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:102: username=username) On 2017/01/13 at 14:44:14, sullivan wrote: > Don't you need to set the id of the table_config in order to look it up by key later? I look it up by querying for name, but I should probably make the id be name to enforce uniqueness. https://codereview.chromium.org/2622303003/diff/80001/dashboard/dashboard/mod... dashboard/dashboard/models/table_config.py:106: ### PROBABLY SHOULD VERIFY JSON OF TABLE LAYOUT On 2017/01/13 at 14:44:14, sullivan wrote: > I would move this comment to line 96, and it should be in the correct style: > > # TODO(jessimb): verify JSON of table layout. Oops that was a comment for myself to remind me to do it. Fixed.
Description was changed from ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-00bab161-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ========== to ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-4fefa630-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ==========
Description was changed from ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-4fefa630-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ========== to ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-156d874e-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ==========
lgtm Just one more test fix inline then fine to submit. https://codereview.chromium.org/2622303003/diff/120001/dashboard/dashboard/cr... File dashboard/dashboard/create_health_report_test.py (right): https://codereview.chromium.org/2622303003/diff/120001/dashboard/dashboard/cr... dashboard/dashboard/create_health_report_test.py:69: }, status=403) Please also check nothing got added to the datastore here!
The CQ bit was checked by jessimb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, sullivan@chromium.org Link to the patchset: https://codereview.chromium.org/2622303003/#ps140001 (title: "updated a test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/13 at 22:48:56, sullivan wrote: > lgtm > > Just one more test fix inline then fine to submit. > > https://codereview.chromium.org/2622303003/diff/120001/dashboard/dashboard/cr... > File dashboard/dashboard/create_health_report_test.py (right): > > https://codereview.chromium.org/2622303003/diff/120001/dashboard/dashboard/cr... > dashboard/dashboard/create_health_report_test.py:69: }, status=403) > Please also check nothing got added to the datastore here! Thanks so much for all your help with this!
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484349253688860,
"parent_rev": "acf4c71e76aa335dd951bcfbfd8ff83a169f3f4d", "commit_rev":
"af5154d43a513b9977fef08e3c79dddf8f9c6567"}
Message was sent while issue was closed.
Description was changed from ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-156d874e-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 ========== to ========== Allows a user to create_health_reports. Creates the /create_health_report endpoint allowing for the upload of table configs. This is a step in the /speed_releasing effort. If you want to put something in the datastore, feel free to try so here: https://dev-jessimb-156d874e-dot-chromeperf.appspot.com/create_health_report BUG=catapult:#3141 Review-Url: https://codereview.chromium.org/2622303003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
