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.

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

Add tests for Hashbang comments #1983

Closed
wants to merge 3 commits into from
Closed

Add tests for Hashbang comments #1983

wants to merge 3 commits into from

Conversation

@bmeck
Copy link
Member

@bmeck bmeck commented Dec 3, 2018

This adds tests around https://github.com/tc39/proposal-hashbang which is at Stage 3 but has not been merged nor has an esid. I tried to figure out if templates could work, but I was unsure about how whitespace worked with them so I left them as standalone files.

@@ -0,0 +1,10 @@
/*---
Copy link
Member

@leobalter leobalter Dec 3, 2018

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.

Loading

Copy link
Member

@leobalter leobalter Dec 3, 2018

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.

Loading

Copy link
Member Author

@bmeck bmeck Dec 3, 2018

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 #!?

Loading

Copy link
Member

@leobalter leobalter Dec 3, 2018

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

Loading

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

@leobalter leobalter Dec 3, 2018

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

Loading

Copy link
Member

@leobalter leobalter Dec 3, 2018

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() {#!
}

Loading

Copy link
Member Author

@bmeck bmeck Dec 3, 2018

Choose a reason for hiding this comment

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

seems fine

Loading

#! SingleLineCommentChars[opt]
---*/

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

@leobalter leobalter Dec 3, 2018

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?

Loading

Copy link
Member Author

@bmeck bmeck Dec 3, 2018

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"?

Loading

Copy link
Member

@leobalter leobalter Dec 3, 2018

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

Loading

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

@leobalter leobalter Dec 3, 2018

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

Loading

HashbangComment::
#! SingleLineCommentChars[opt]
flags: [module]
---*/
Copy link
Member

@leobalter leobalter Dec 3, 2018

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

Loading

info: |
HashbangComment::
#! SingleLineCommentChars[opt]
flags: [module]
Copy link
Member

@leobalter leobalter Dec 3, 2018

Choose a reason for hiding this comment

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

remove?

Loading

@@ -0,0 +1,10 @@
#! these characters should be treated as a comment
Copy link
Member

@leobalter leobalter Dec 3, 2018

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...

Loading

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

@leobalter leobalter Dec 3, 2018

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.

Loading

@@ -0,0 +1,10 @@
#!
Copy link
Member

@leobalter leobalter Dec 3, 2018

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 !

Loading

Copy link
Member

@mathiasbynens mathiasbynens Jan 28, 2019

Choose a reason for hiding this comment

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

Loading

Copy link
Member

@leobalter leobalter Jan 28, 2019

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

Loading

#! SingleLineCommentChars[opt]
---*/

assert.sameValue(eval('#!\n'), undefined);
Copy link
Member

@leobalter leobalter Dec 3, 2018

Choose a reason for hiding this comment

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

add here assertions to assert they are only available at the beginning?

assert.throws(SyntaxError, () => eval(' #!\n'), 'whitespace');
assert.throws(SyntaxError, () => eval('\n#!\n'), 'new line');

Loading

@leobalter
Copy link
Member

@leobalter leobalter commented Dec 10, 2018

I'll work on the linter this week to not fail tests like this as we need the hashbang spec to be fully observed,

The PR is already good to land as it is otherwise.

Loading

@leobalter leobalter self-assigned this Dec 15, 2018
@anba
Copy link
Contributor

@anba anba commented Dec 18, 2018

test/language/comments/hashbang-encoded-bang.js
test/language/comments/hashbang-encoded-hash.js
test/language/comments/hashbang-encoded-hashbang.js

  • Missing negative marker in yaml descriptor.

test/language/comments/hashbang-function-body.js

  • Missing var (or let or const) before ctor making this test throw a ReferenceError in strict mode.

Loading

@leobalter
Copy link
Member

@leobalter leobalter commented Dec 18, 2018

@bmeck besides @anba feedback, would you mind adding two things?

  • the 2 copyright lines right before every file's /*--- (the frontmatter starter)?
  • In every file that has anything before the copyright lines, please add their names to the lint.exceptions (if you don't rebase your branch: lint.whitelist)?

examples to the exceptions list:

test/language/comments/hashbang-encoded-bang.js LICENSE
test/language/comments/hashbang-encoded-hash.js LICENSE
test/language/comments/hashbang-not-empty.js LICENSE
...

WDYT having these files in a separate folder? both test/language/hashbang/ or test/language/comments/hashbang/ works for me. I slightly prefer the first option, thou.

This way we can have a nice report of coverage only for hashbang through the folders organization and this should also allow some test runners to observe the full folder while the feature is still not yet implemented.

Loading

@leobalter
Copy link
Member

@leobalter leobalter commented Dec 18, 2018

I'm happy to work on the necessary changes here, the only info I would need is the appropriate name to use in the copyright header.

Loading

info: |
HashbangComment::
#! SingleLineCommentChars[opt]
flags: [raw]
Copy link
Contributor

@caitp caitp Jan 10, 2019

Choose a reason for hiding this comment

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

Could you please add feature front matter to each test?

features: [hashbang]

presumably

Loading

Copy link
Member

@mathiasbynens mathiasbynens Jan 28, 2019

Choose a reason for hiding this comment

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

(and update features.txt accordingly)

Loading

@@ -0,0 +1,10 @@
#\u0021
Copy link
Contributor

@caitp caitp Jan 10, 2019

Choose a reason for hiding this comment

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

I think this is also incompatible with https://github.com/tc39/test262/blob/master/tools/packaging/parseTestRecord.py (which we use in v8) --- we might definitely need to adjust yamlPattern add a fallback case to get YAML from the tail of the file to handle this

Loading

/*---
esid: pending
description: >
Hashbang comments should not be interpretted and should not generate DirectivePrologues.
Copy link
Contributor

@jswalden jswalden Jan 11, 2019

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.

Loading

/*---
esid: pending
description: >
Hashbang comments should not be allowed to have encoded characters
Copy link
Contributor

@caitp caitp Jan 14, 2019

Choose a reason for hiding this comment

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

Given the description, shouldn't this be a negative test expecting a SyntaxError?

Loading

@adrianheine
Copy link
Contributor

@adrianheine adrianheine commented Jan 27, 2019

Support for this in test262-parser: smikes/test262-parser#14.

Loading

HashbangComment::
#! SingleLineCommentChars[opt]
flags: [raw]
---*/
Copy link
Contributor

@adrianheine adrianheine Jan 27, 2019

Choose a reason for hiding this comment

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

Should be a negative test.

Loading

HashbangComment::
#! SingleLineCommentChars[opt]
flags: [raw]
---*/
Copy link
Contributor

@adrianheine adrianheine Jan 27, 2019

Choose a reason for hiding this comment

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

Should be a negative test.

Loading

brn pushed a commit to brn/v8 that referenced this issue Jan 30, 2019
Implements https://tc39.github.io/proposal-hashbang/, which simply
ignores the first line of a source file if it begins with '#!'
(U+0023 U+0021).

The test cases are influenced by
tc39/test262#1983, which have not been pulled
into test262 local-tests due to issues with parseTestRecord.

BUG=v8:8523
R=gsathya@chromium.org, adamk@chromium.org, littledan@chromium.org

Change-Id: I4ae40222298de768a170c7a1d45fec118ed5713c
Reviewed-on: https://chromium-review.googlesource.com/c/1409527
Commit-Queue: Caitlin Potter <caitp@igalia.com>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58838}
@leobalter leobalter mentioned this pull request Feb 5, 2019
@leobalter leobalter closed this Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants