Quadratic Blowup test is broken

RESOLVED FIXED in 1.0

Status

L20n
JS Library
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: stas, Assigned: gandalf)

Tracking

unspecified
x86_64
All

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
It should be moved to tests/parser/insecure.  One of my patches in bug 918655 made that change, but maybe it wasn't git-added prior to landing?
(Reporter)

Updated

4 years ago
Flags: needinfo?(gandalf)
(Reporter)

Comment 1

4 years ago
Gandalf, did you have a reason for not landing it or was it just an omission?
(Assignee)

Comment 2

4 years ago
The test was removed by your patch, not readded. No reason, I believe I was confused as I was trying to apply your version of the patch against my branch.
Flags: needinfo?(gandalf)
(Reporter)

Comment 3

4 years ago
It was readded in https://bugzilla.mozilla.org/attachment.cgi?id=811531&action=diff#a/tests/lib/parser/insecure/dos.js_sec2.  Can you please prepare a patch that adds it on master?
(Assignee)

Updated

4 years ago
Assignee: nobody → gandalf
Target Milestone: --- → 1.0
(Assignee)

Comment 4

4 years ago
Stas, it seems that the file is on master, here: https://github.com/l20n/l20n.js/blob/master/tests/lib/compiler/insecure/dos.js

Should I close this bug?
Flags: needinfo?(stas)
(Reporter)

Comment 5

4 years ago
No.

I think that what we need is to split that file into two tests:

  tests/lib/compiler/insecure/dos.js with the Billion Laughs test
  tests/lib/parser/insecure/dos.js with the Quadratic Blowup test

That's what my patch that I mentioned in comment 3 did.
Flags: needinfo?(stas)
(Assignee)

Comment 6

4 years ago
Created attachment 820388 [details] [diff] [review]
923461.patch
Attachment #820388 - Flags: review?(stas)
(Reporter)

Comment 7

4 years ago
Comment on attachment 820388 [details] [diff] [review]
923461.patch

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

It looks like this is not the right patch?
Attachment #820388 - Flags: review?(stas) → review-
(Assignee)

Comment 8

4 years ago
Created attachment 820611 [details] [diff] [review]
bug923768.diff

Gosh... today is not a good day for me.

Apologies again Stas.
Attachment #820388 - Attachment is obsolete: true
Attachment #820611 - Flags: review?(stas)
(Reporter)

Comment 9

4 years ago
Comment on attachment 820611 [details] [diff] [review]
bug923768.diff

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

No worries, and thanks for fixing this!
Attachment #820611 - Flags: review?(stas) → review+
(Assignee)

Comment 10

4 years ago
https://github.com/l20n/l20n.js/commit/59039cd21a4f51d86d36b7143be5f8986f32bc4e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.