Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests for Hashbang comments #1983

Closed
wants to merge 3 commits into from
Closed

Add tests for Hashbang comments #1983

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ddcec6
Add tests for Hashbang comments
bfarias-godaddy Dec 3, 2018
1f84c8f
nits
bfarias-godaddy Dec 3, 2018
ea92b44
nits
bfarias-godaddy Dec 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions test/language/comments/hashbang-eval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I didn't have to say that, but we need to have the 2 lines of copyright on each test file.

I'm sorry this is a thing we'd need to get fixed in a meeting. Thanks for understanding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate this requirement, btw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it fine to have them below the #!?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! I only need to "fix" the linter

esid: pending
description: >
Hashbang comments should be available in Script evaluator contexts.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]
---*/

eval('#!\n');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert this completes to undefined?

Extra: maybe add another test file to evaluate custom completions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you define "custom completions"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

assert.sameValue(eval('#!\n1`'), 1)
assert.sameValue(eval('#!2`'), undefined)

custom is a bad word, but any completion value that doesn't come from the hashbang comment

23 changes: 23 additions & 0 deletions test/language/comments/hashbang-function-constructor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*---
esid: pending
description: >
Hashbang comments should not be allowed in function evaluator contexts.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]
flags: [module]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

---*/
const AsyncFunction = (async function (){}).constructor;
const GeneratorFunction = (function *(){}).constructor;
const AsyncGeneratorFunction = (async function *(){}).constructor;
for (ctor of [
Function,
AsyncFunction,
GeneratorFunction,
AsyncGeneratorFunction,
]) {
assert.throws(SyntaxError, () => ctor('#!\n_',''));
assert.throws(SyntaxError, () => ctor('#!\n_'));
assert.throws(SyntaxError, () => new ctor('#!\n_',''));
assert.throws(SyntaxError, () => new ctor('#!\n_'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

  assert.throws(SyntaxError, () => ctor('#!\n_',''), `${ctor.name} #1`);
  assert.throws(SyntaxError, () => ctor('#!\n_'), `${ctor.name} #2`);
  assert.throws(SyntaxError, () => new ctor('#!\n_',''), `${ctor.name} #3`);
  assert.throws(SyntaxError, () => new ctor('#!\n_'), `${ctor.name} #4`);

WDYT? this helps mapping the assertions to whoever is consuming this test file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add assertions for hashbangs placed at the first part of a function block?

function fn() {#!
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine

}
10 changes: 10 additions & 0 deletions test/language/comments/hashbang-module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add other test files verifying these characters can't be escaped? one for # and another for !

Copy link
Member

@mathiasbynens mathiasbynens Jan 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s try multiple types of escapes: https://github.com/v8/v8/blob/effb7ad728cf13da143fae41b03b0525f4ff65d5/test/cctest/test-parsing.cc#L11446-L11455

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! I'll work on some follow ups for this PR and this is a good material

/*---
esid: pending
description: >
Hashbang comments should be allowed in Modules.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]
flags: [module]
---*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the flags: [raw] so test runners won't prepend the harness files

15 changes: 15 additions & 0 deletions test/language/comments/hashbang-multi-line-comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/*
these characters should not be considered within a comment
*/
/*---
esid: pending
description: >
Hashbang comments should not interpret multi-line comments.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, this test is so fun!

please add the flags: [raw] so test runners won't add harness files

10 changes: 10 additions & 0 deletions test/language/comments/hashbang-no-line-separator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*---
esid: pending
description: >
Hashbang comments should not require a newline afterwards
info: |
HashbangComment::
#! SingleLineCommentChars[opt]
---*/

eval('#!');
10 changes: 10 additions & 0 deletions test/language/comments/hashbang-not-empty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#! these characters should be treated as a comment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely need to change the linter to not fail for this. I believe it will say you're missing the copyright lines here...

/*---
esid: pending
description: >
Hashbang comments should be allowed in Scripts and should not be required to be empty.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]
---*/

14 changes: 14 additions & 0 deletions test/language/comments/hashbang-preceding-directive-prologue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
"use strict"
#!
/*---
esid: pending
description: >
Hashbang comments should only be allowed at start of source texts and should not be preceded by DirectivePrologues.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
13 changes: 13 additions & 0 deletions test/language/comments/hashbang-preceding-empty-statement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
;#!
/*---
esid: pending
description: >
Hashbang comments should only be allowed at the start of source texts and should not be preceded by empty statements.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
14 changes: 14 additions & 0 deletions test/language/comments/hashbang-preceding-hashbang.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!
#!
/*---
esid: pending
description: >
Hashbang comments should only be allowed at the start of source texts and should not be preceded by Hashbang comments.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
14 changes: 14 additions & 0 deletions test/language/comments/hashbang-preceding-line-comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//
#!
/*---
esid: pending
description: >
Hashbang comments should only be allowed at the start of source texts and should not be preceded by line comments.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same case of raw mode, please add this to the other files accordingly.

14 changes: 14 additions & 0 deletions test/language/comments/hashbang-preceding-multi-line-comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
*/#!
/*---
esid: pending
description: >
Hashbang comments should only be allowed at the start of source texts and should not be preceded by multi-line comments.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
13 changes: 13 additions & 0 deletions test/language/comments/hashbang-preceding-whitespace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/bin/sh
/*---
esid: pending
description: >
Hashbang comments should only be allowed at the start of source texts and should not be preceded by whitespace.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
15 changes: 15 additions & 0 deletions test/language/comments/hashbang-statement-block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*---
esid: pending
description: >
Hashbang comments should only be allowed at the start of source texts and should not be allowed within blocks.
info: |
HashbangComment::
#! SingleLineCommentChars[opt]

negative:
phase: parse
type: SyntaxError
---*/
{
#!
}
11 changes: 11 additions & 0 deletions test/language/comments/hashbang-use-strict.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!"use strict"
/*---
esid: pending
description: >
Hashbang comments should not be interpretted and should not generate DirectivePrologues.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Hashbang comments should not be interpretted and should not generate DirectivePrologues.
Hashbang comments should not be interpreted and should not generate DirectivePrologues.

info: |
HashbangComment::
#! SingleLineCommentChars[opt]
---*/

with ({}) {}