Make the jit-test runner look for a per-directory directive file

RESOLVED FIXED in Firefox 52

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Spun out as future work from https://bugzilla.mozilla.org/show_bug.cgi?id=1232205#c33 and previous comments:

We would like the test runner to have the option of not just scanning a file for directives in its header (ie, "|jit-test|" ...) but for it to recognize a per-directory file containing such directives, a strawman name for that file is directives.txt.  The meaning of the file would be that the directives in the file would be merged with the directives in each file in the directory.

(I'm on the fence regarding the file applying to subdirectories, I'm thinking probably not.)

Updated

2 years ago
Priority: -- → P3
(Assignee)

Updated

2 years ago
Priority: P3 → P5
(Assignee)

Updated

2 years ago
Blocks: 1313114
(Assignee)

Comment 1

2 years ago
P2 to avoid a priority inversion.
Priority: P5 → P2
(Assignee)

Comment 2

2 years ago
Created attachment 8808575 [details] [diff] [review]
bug1277770-jit-test-directives-from-file.patch

Looks for directives.txt in the directory of the test case, and extracts a jit-test directive out of that if there's one.  It is then catenated with whatever's in the test file.  The directive is cached in the test runner, though I don't know if that really is necessary.
Attachment #8808575 - Flags: review?(jcoppeard)
(Assignee)

Comment 3

2 years ago
BTW, examples of how this is used will appear shortly on bug 1313114.
Comment on attachment 8808575 [details] [diff] [review]
bug1277770-jit-test-directives-from-file.patch

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

This looks good.

Maybe you should check if there are extra lines in the directives file that will get ignored though.

::: js/src/tests/lib/jittests.py
@@ +180,5 @@
> +        # the first line is considered.
> +
> +        dir_name = os.path.dirname(path)
> +        if not dir_name.endswith('/'):
> +            dir_name += '/'

You don't need to add the slash here...

@@ +186,5 @@
> +        dir_meta = ''
> +        if dir_name in cls.Directives:
> +            dir_meta = cls.Directives[dir_name]
> +        else:
> +            meta_file_name = dir_name + "directives.txt"

...if you use os.path.join here.

@@ +191,5 @@
> +            if os.path.exists(meta_file_name):
> +                line = open(meta_file_name).readline()
> +                i = line.find(cls.COOKIE)
> +                if i != -1:
> +                    dir_meta = ';' + line[i + len(cls.COOKIE):].strip('\n')

Please factor out this parsing code into a separate function to avoid repeating it below.

Also it's probably more readable to call split() on the string than using find() and subscript to do this.
Attachment #8808575 - Flags: review?(jcoppeard) → review+

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/788c788561b4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.