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

getFnName unit tests ADDED #10941

Merged
merged 1 commit into from
Feb 15, 2018
Merged

Conversation

dragthor
Copy link
Contributor

Foundation.getFnName() unit tests added.

Verified

This commit was signed with the committer’s verified signature.
Byron Sebastian Thiel
Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

👍 LGTM, Thanks @dragthor

@ncoden
Copy link
Contributor

ncoden commented Feb 14, 2018

@dragthor I seen your comment in #8311 (comment). Do you plan to add any others tests in this PR ?

You can open/rename a PR with the [WIP] tag.

@dragthor
Copy link
Contributor Author

Separate PR.

@ncoden
Copy link
Contributor

ncoden commented Feb 15, 2018

Ok then...

@ncoden ncoden merged commit 4537944 into foundation:develop Feb 15, 2018
@ncoden
Copy link
Contributor

ncoden commented Feb 15, 2018

Note: tests are being repaired in #10943.
I tested in local, everything works great.

@ncoden
Copy link
Contributor

ncoden commented Feb 15, 2018

@dragthor See #10944

I seen that failed in the tests. These tests were added today in #10941. It worked well with phantomjs.
I'm not sure of what we should expect in this case. @dragthor do you have an opinion on this ?

@ncoden ncoden mentioned this pull request Feb 15, 2018
3 tasks
@dragthor
Copy link
Contributor Author

dragthor commented Feb 16, 2018

Let me figure this out.

Edge ""
IE11 ""
phantomjs ""
FF "B"
Chrome "B"
Safari 11 "B"
node v8.9.1 "B"

@dragthor
Copy link
Contributor Author

I need to remove the unnamed function expression test case.

@DanielRuf
Copy link
Contributor

DanielRuf commented Feb 16, 2018

Seems some (newer) browsers use the variable name for automatic namespacing.

Is it safe to remove the unnamed function expression test case?

@ncoden
Copy link
Contributor

ncoden commented Feb 16, 2018

As there is no consistent behavior we can expect (but just "we found a name", or "we were not able to get a name", and unless we find a polyfill (for new or old browsers), we should not test this case.

ncoden added a commit to ncoden/foundation-sites that referenced this pull request Feb 26, 2018

Verified

This commit was signed with the committer’s verified signature.
Byron Sebastian Thiel
ncoden pushed a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018

Verified

This commit was signed with the committer’s verified signature.
Byron Sebastian Thiel
…6.5.0

5137351 getFnName unit tests

Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants