test(*): run class-related tests everywhere; fix eval syntax #15967
Conversation
1. Wrap an evaled class definition in parens; previously they weren't; the test wasn't failing only because it was disabled everywhere outside of Chrome and Chrome <59 incorrectly accepted such input. 2. There's no reason to restrict class-related tests just to Chrome; now they run in every browser that supports ES6 classes. The classes support test was modified to check not only if a class definition parses but also if it stringifies correctly which is required by AngularJS. This restriction disables class-related tests in current Firefox (53) but will work in v55 or newer.
There was a problem hiding this comment.
My only concern is the following:
In case that our supportTests start returning false (so the tests are skipped), we might break the functionality but won't see any tests failures.
I would feel more comfortable if we had some way to essentially verify the supportTests. E.g. if we know that Chrome should support all features, have tests that verify the if (/chrome/.test(navigator.userAgent)) support.* === true. (Open to better ideas too 😃)
|
@gkalpak I tend to agree. While in code shipped to browsers support tests are much, much better than UA-sniffing (so that unknown environments with proper support works), in tests it may be better to UA-sniff to make sure that our assertions are correct. Note that currently we have the EDIT (I misread the comment): your suggestion to verify support test results in known browsers actually sounds better to me than any hardcoding in known enviros! I'll add that soon. |
|
We could also create a "blacklist" of browsers we know will not support classes (basically IE). That way, every new browser is automatically assuned to support classes |
|
It's not just IE, also old Safari/iOS (which we'll be testing for a while) and we might want to try running the tests on the Android Browser 4.0-4.3. For now it seems better IMO to just assert the results are what we expect them to be as we do in jQuery: https://github.com/jquery/jquery/blob/3.2.1/test/unit/support.js#L61-L235 |
|
I'd like to land this PR as-is, the new tests verifying support tests results seem like a separate thing (I can submit a PR for that shortly). |
|
I am fine adding the support tests in a separate PR. |
|
Me too! |
1. Wrap an evaled class definition in parens; previously they weren't; the test wasn't failing only because it was disabled everywhere outside of Chrome and Chrome <59 incorrectly accepted such input. 2. There's no reason to restrict class-related tests just to Chrome; now they run in every browser that supports ES6 classes. The classes support test was modified to check not only if a class definition parses but also if it stringifies correctly which is required by AngularJS. This restriction disables class-related tests in current Firefox (53) but will work in v55 or newer. Closes #15967
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Test fixes.
What is the current behavior? (You can also link to an open issue here)
There was a syntax error in class-related tests; the test wasn't failing only because it was disabled everywhere outside of Chrome and Chrome <59 incorrectly accepted it.
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements
Other information: