Update parse_scalars.py to make use of extended validator functionality

RESOLVED FIXED in Firefox 61

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: adbugger, Assigned: diorahman, Mentored)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [lang=python][good-next-bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
The shared_telemetry_utils.py file was edited and the ParserError class was extended in bug 1401612. The validator now outputs multiple errors at a time and all ParserErrors have been separated into immediately fatal and eventually fatal. The parse_histograms.py parser has already been updated, this bug is to update the parse_scalars.py file.
(Reporter)

Updated

a year ago
See Also: → 1426460
(Reporter)

Updated

a year ago
See Also: → 1401612
(Reporter)

Updated

a year ago
Has STR: --- → irrelevant
Mentor: chutten, alessio.placitelli
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [lang=python][good-next-bug]
Hello I would like to work on this bug.?
Can I get started.
You certainly may! To start, I recommend reading bug 1401612 paying close attention to the patch that was eventually submitted. Your work will be to do a very similar kind of thing, but for the parse_scalars.py file.

If you have any questions, please do not hesitate to ask!
Assignee: nobody → ashish1500616
Status: NEW → ASSIGNED
(In reply to Chris H-C :chutten from comment #2)
> You certainly may! To start, I recommend reading bug 1401612 paying close
> attention to the patch that was eventually submitted. Your work will be to
> do a very similar kind of thing, but for the parse_scalars.py file.
> 
> If you have any questions, please do not hesitate to ask!

I am new to the organization and I would need your help.
(In reply to Chris H-C :chutten from comment #2)
> You certainly may! To start, I recommend reading bug 1401612 paying close
> attention to the patch that was eventually submitted. Your work will be to
> do a very similar kind of thing, but for the parse_scalars.py file.
> 
> If you have any questions, please do not hesitate to ask!

I am new to the organization and I would need your help. What needs to be done to fix the issue?
What is extended validator functionality?How can I go about fixing the issue?
Sorry about the delay. I work for Mozilla so you'll generally not see much activity from me on weekends :)

To fix this bug you'll be using the new ParserError functionality "handle_later()" for most of the errors that happen in the file toolkit/components/telemetry/parse_scalars.py

Any errors that should fail the build immediately would be "handle_now()" instead.

parse_scalars.py is run during Firefox's build step. These changes will make it so that a single build step will reveal all the errors in the changes you made, instead of having the build fail immediately with only one problem listed.

Does this make sense?
Thanks :chutten ,Yes now it make some sense to me.I will start working on it .
Hello! Do you need any help? Are you still working on this?
Flags: needinfo?(ashish1500616)
Flags: needinfo?(chutten)
Marking unassigned due to inactivity.
Assignee: ashish1500616 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(chutten)
Flags: needinfo?(ashish1500616)
(Assignee)

Comment 10

a year ago
(Assignee)

Updated

a year ago
Assignee: nobody → diorahman
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Attachment #8955749 - Flags: review?(chutten)
Comment on attachment 8955749 [details] [diff] [review]
Make use of extended validator functionality

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

Thank you for your patch!

In what ways did you test that this was working? The way it is written, I don't see any handle_now or handle_later calls that would report errors. Did you create a malformed scalar definition to ensure that these errors are being reported properly?
Attachment #8955749 - Flags: review?(chutten)
(Assignee)

Comment 12

a year ago
Hi Chris!

It seems I missed those points on checking the _handle_now or handle_later calls_. Could you help me by elaborating it again? Thanks!
Flags: needinfo?(chutten)
I covered the basics in comment #6, but put another way:

parse_scalars.py is run during the build step.
At this point, it could find some errors in the Scalars.yaml file
Currently, after we find a single error, we stop and print it
We recently added the ability to handle errors later, so we can find most or all of the errors in one build step
To signify which errors we can handle later, we use .handle_later. These errors will print at the very end of the parse.
If the error arose because we can't actually process any further (like, say, the file is missing) we use .handle_now to print all the accumulated errors immediately.

A patch to fix this bug will mark most of these ParserErrors as handle_later.
To test that your patch is correct, you will need to make some malformed Scalar definitions in Scalars.yaml for the errors to find. This will just be local testing, as I'm not sure we have the framework built just yet for you to be able to write automated tests in the tests/python dir. But you can take a look if you're keen to.

Does this help?
Flags: needinfo?(chutten)
(Assignee)

Comment 14

a year ago
(Assignee)

Updated

a year ago
Attachment #8955749 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8955749 - Flags: review?(chutten)
Attachment #8955749 - Flags: review?(chutten)
Comment on attachment 8957046 [details] [diff] [review]
Make use of extended validator functionality

Going to assume you meant to put the r? on the new patch instead of the old one :)
Attachment #8957046 - Flags: review?(chutten)
Comment on attachment 8957046 [details] [diff] [review]
Make use of extended validator functionality

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

This looks good! How did you test your change?
Attachment #8957046 - Flags: review?(chutten) → review+
(Assignee)

Comment 17

a year ago
(In reply to Chris H-C :chutten from comment #16)
> Comment on attachment 8957046 [details] [diff] [review]
> Make use of extended validator functionality
> 
> Review of attachment 8957046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good! How did you test your change?

Hi Chris, I manually introduce the error to the Scalars.yml. Yeah, I was thinking to automate it but was not so sure.
(Assignee)

Comment 18

a year ago
(In reply to Chris H-C :chutten from comment #15)
> Comment on attachment 8957046 [details] [diff] [review]
> Make use of extended validator functionality
> 
> Going to assume you meant to put the r? on the new patch instead of the old
> one :)

Ah yeah, sorry for that.
(In reply to Dhi Aurrahman from comment #17)
> (In reply to Chris H-C :chutten from comment #16)
> > Comment on attachment 8957046 [details] [diff] [review]
> > Make use of extended validator functionality
> > 
> > Review of attachment 8957046 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good! How did you test your change?
> 
> Hi Chris, I manually introduce the error to the Scalars.yml. Yeah, I was
> thinking to automate it but was not so sure.

We do have one python test, but it might not be within the scope of this "good next bug" to write one for parse_scalars.

However, that would be an excellent next bug to try, if you're interested. You could file the bug yourself and then use the test_histogramtools_non_strict.py test as an example?
Keywords: checkin-needed
(Assignee)

Comment 20

a year ago
(In reply to Chris H-C :chutten from comment #19)
> (In reply to Dhi Aurrahman from comment #17)
> > (In reply to Chris H-C :chutten from comment #16)
> > > Comment on attachment 8957046 [details] [diff] [review]
> > > Make use of extended validator functionality
> > > 
> > > Review of attachment 8957046 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This looks good! How did you test your change?
> > 
> > Hi Chris, I manually introduce the error to the Scalars.yml. Yeah, I was
> > thinking to automate it but was not so sure.
> 
> We do have one python test, but it might not be within the scope of this
> "good next bug" to write one for parse_scalars.
> 
> However, that would be an excellent next bug to try, if you're interested.
> You could file the bug yourself and then use the
> test_histogramtools_non_strict.py test as an example?

Interesting, let me take a look. But since I have no previous experience in writing complicated python scripts, it will take a while.

Comment 21

a year ago
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13163586e52b
Make use of extended validator functionality r=chutten
Keywords: checkin-needed
Please back it out, :gaston.

Dhi, you can run `./mach lint` locally to ensure your change adheres to the style guidelines.
Well i ain't no sherriff, so i think dluca should do this backout and repush bug 1442746 while here...
Backed out for flake8 linting failure:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35c35cbe59f9201ca16b7b0ff1a1ef77a0a26bf0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167002221&repo=mozilla-inbound
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:62:86 | statement ends with a semicolon (E703)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:69:100 | line too long (107 > 99 characters) (E501)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:75:100 | line too long (108 > 99 characters) (E501)

Please fix the issues and submit an updated patch.
Flags: needinfo?(diorahman)
(Assignee)

Comment 26

a year ago
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #25)
> Backed out for flake8 linting failure:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 35c35cbe59f9201ca16b7b0ff1a1ef77a0a26bf0
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=167002221&repo=mozilla-
> inbound
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:
> 62:86 | statement ends with a semicolon (E703)
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:
> 69:100 | line too long (107 > 99 characters) (E501)
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:
> 75:100 | line too long (108 > 99 characters) (E501)
> 
> Please fix the issues and submit an updated patch.

Will do
Flags: needinfo?(diorahman)
(Assignee)

Updated

a year ago
Attachment #8957724 - Flags: review?(chutten)
(Assignee)

Updated

a year ago
Attachment #8957046 - Attachment is obsolete: true
Comment on attachment 8957724 [details] [diff] [review]
Make use of extended validator functionality

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

Did you use ./mach lint to make sure we got them all?
Attachment #8957724 - Flags: review?(chutten) → review+
(Assignee)

Comment 29

a year ago
(In reply to Chris H-C :chutten from comment #28)
> Comment on attachment 8957724 [details] [diff] [review]
> Make use of extended validator functionality
> 
> Review of attachment 8957724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you use ./mach lint to make sure we got them all?

Yes Chris, sorry for the mistake. 

✖ 0 problems (0 errors, 0 warnings)
Excellent, just checking :)
Keywords: checkin-needed

Comment 31

a year ago
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/782ec59a7264
Make use of extended validator functionality r=chutten
Keywords: checkin-needed

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/782ec59a7264
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.