Closed Bug 1064218 Opened 5 years ago Closed 5 years ago

Detect syntax failures in Loop's unit tests

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
1

Tracking

(firefox35 wontfix, firefox36 fixed, firefox37 fixed)

RESOLVED FIXED
mozilla37
Iteration:
37.1
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed
firefox37 --- fixed
Blocking Flags:
backlog Fx37+

People

(Reporter: standard8, Assigned: standard8)

Details

(Whiteboard: [tech-debt])

Attachments

(1 file, 1 obsolete file)

Loop's unit tests currently have an issue whereby if there is a syntax error, we don't detect it. We should attempt to do that, if possible, to help avoid errors in our tests that go undetected.

I'm attaching a PoC patch that would work towards some of this. It currently relies on a hard-coded list of titles, which I'd prefer not to do. However, we obviously can't self-build the list, but also, mocha doesn't really have enough facilities to detect new titles without adding every describe to a list.

So for now, it relies on the hard-coded list, but this seems better than nothing (and we can add docs to the script inclusion to say to update the expectedTitles list).

The automated scripts should be able to be updated to check for "Syntax Error" versus "Complete".

Looking for feedback before I push this any further.
See comment 0.
Attachment #8485668 - Flags: feedback?(nperriault)
Attachment #8485668 - Flags: feedback?(dmose)
Comment on attachment 8485668 [details] [diff] [review]
PoC Attempt to detect test syntax failures

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

::: browser/components/loop/test/desktop-local/index.html
@@ +48,5 @@
>    <script src="conversation_test.js"></script>
>    <script src="panel_test.js"></script>
>    <script>
>      mocha.run(function () {
> +      var expectedTitles = ["loop.Client", "loop.panel", "loop.conversation"];

That means we must add new titles every time they're added to the test files, right? Seems unpractical and highly error prone to me.

Couldn't window.onerror be what we really need here? We'd obviously have to ensure syntax errors are caught by it first. Also, this global error handler should probably be defined before any script import being performed.
Attachment #8485668 - Flags: feedback?(nperriault) → feedback-
(In reply to Nicolas Perriault (:NiKo`) from comment #2)
> That means we must add new titles every time they're added to the test
> files, right? Seems unpractical and highly error prone to me.

That's true but only top-level titles, which should just be whenever new files are added.

The intent is to fix the issue of our don't even have a chance of detecting it for existing tests, to have a change of detecting it for existing tests - but with an smaller risk of forgetting to add new files...

However...

> Couldn't window.onerror be what we really need here? We'd obviously have to
> ensure syntax errors are caught by it first. Also, this global error handler
> should probably be defined before any script import being performed.

this would be ideal, if it works, I'll try it out sometime.
Comment on attachment 8485668 [details] [diff] [review]
PoC Attempt to detect test syntax failures

I like the windows.onerror theory too...
Attachment #8485668 - Flags: feedback?(dmose)
Blocks: 1064900
No longer blocks: 1064900
Whiteboard: [tech-debt]
backlog: --- → Fx36+
backlog: Fx36+ → Fx37+
Assignee: nobody → standard8
We're hitting a failure again currently and not detecting it. So I worked up a new patch based on the window.error idea.
Attachment #8530739 - Flags: review?(nperriault)
Attachment #8485668 - Attachment is obsolete: true
Comment on attachment 8530739 [details] [diff] [review]
Detect syntax failures in Loop's unit tests.

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

Nice!

::: browser/components/loop/test/desktop-local/index.html
@@ +16,5 @@
>    <div id="fixtures"></div>
> +  <script>
> +    var syntaxError;
> +    window.addEventListener("error", function(error) {
> +     syntaxError = error;

Nit: I suspect this isn't specifically a syntax error, but could be any exception not caught by mocha. If that's verified, I'd go s/syntaxError/uncaughtError
Attachment #8530739 - Flags: review?(nperriault) → review+
https://hg.mozilla.org/integration/fx-team/rev/8c447579ec2f
Iteration: --- → 37.1
Points: --- → 1
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/8c447579ec2f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8530739 [details] [diff] [review]
Detect syntax failures in Loop's unit tests.

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Syntax errors in Loop tests could be missed

[Describe test coverage new/current, TBPL]: On m-c for a week or so.

[Risks and why]: No risk basically; test-only change to avoid missing a class of test failure.

[String/UUID change made/needed]: none
Attachment #8530739 - Flags: approval-mozilla-beta?
Attachment #8530739 - Flags: approval-mozilla-aurora?
Attachment #8530739 - Flags: approval-mozilla-beta?
Attachment #8530739 - Flags: approval-mozilla-beta+
Attachment #8530739 - Flags: approval-mozilla-aurora?
Attachment #8530739 - Flags: approval-mozilla-aurora+
Target Milestone: mozilla36 → mozilla37
You need to log in before you can comment on or make changes to this bug.