Work on updating specs (and a few useful features) #78
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We now need at least opal 1.0.0 and opal-rspec 0.8.0.alpha1. The following commits will attempt to fix incompatibilities.
Newer opal-rspec releases expect specs to be present in "spec-opal" directory. We want opal-rspec to seek them in a "spec" directory as before.
"async" syntax got dropped in the recent opal-rspec releases. The specs were updated to use promises.
A previous committer noticed that Opal doesn't rescue from JS exceptions. That's true, because JS::Error isn't a subclass of StandardError. This commit should fix the issue.
Element#at. Document#[] is meant to return either an element with a given id, the first element with a given css path or the first element with a given xpath path whichever comes first. The previous implementation assumed that Element#css and Element#xpath return nil if a path is invalid and that was an incorrect assumption: they may throw a JS exception. The new implementation tries to catch such an exception so that the method works as advertised. Same with Element#at except that this one doesn't have a spec.
I don't really know what I'm doing here. I don't expect this to work, but travis itself may give us some idea.
It's quite weird and probably warrants additional inspection from
an Opal developer perspective. In theory Opal should have added a
return statement itself, but it forgot to and that's kind of
interesting. Other (untested) cases all seem to be alright, it's
probably because this expression is quite complicated involving
line breaks and {,} characters. I checked other Event cases like
this that aren't tested and they don't suffer from this bug.
Oh, actually I found out that Event::GamePad was also affected.
As per [1], there's another state that wasn't taken into account and that is "interactive" which is between a DOM load and a full load. We expect Document#ready? (DOM load) to always be true when Document#ready calls back. This was a corner case when it was false. This one caused a delayed test failure. [1] https://developer.mozilla.org/en-US/docs/Web/API/Document/readyState
This one was caused, again, by a too weak exception catch. A JS::Error is not a subclass of a StandardError (just Exception). Expressions in the form of "x rescue nil" are equivalent of "begin x rescue; nil end" which obviously only catch StandardErrors. This commit makes a test page free from exceptions and green like a cuke.
|
I think I can do nothing more about BrowserStack tests. There's probably "BS_AUTHKEY" environment variable missing, that's what I understand from this error message: *** Error: Atleast one argument is required! I tested the BrowserStackLocal application locally and it only triggers this message when there's no key provided. |
This commit implements:
- Element#animate
- Element#fade_in
- Element#fade_out
- Element#fade_toggle
- Element#slide_down
- Element#slide_up
- Element#slide_toggle
I tested those changes with the following example code:
```
<button id='butt1'>Animate</button>
<button id='butt2'>Animate</button>
<div id='xdiv' style='height: 300px; width: 300px; background-color: red'></div>
```
```
$document.ready do
butt1 = $document['butt1']
butt2 = $document['butt2']
xdiv = $document['xdiv']
butt1.on :click do
xdiv.slide_toggle
end
butt2.on :click do
xdiv.fade_toggle
end
end
```
hmdne
changed the title
|
I know I could have broken it all up to individual pull requests, but I hope it doesn't make your work harder (it's much easier for me that way). I try to describe the commits as well as I can. |
No problem, I'll review the commit one by one (as I usually do) it will just take time. I think I'll do multiple reviews so you can get early feedback 👍 |
elia
approved these changes
Sep 19, 2019
- Element#css and Element#xpath should NOT raise exceptions and the rest of code should be updated under this assumption - Exceptions should be caught by "StandardError, JS::Error" and not "Exception"
|
Thank you for this review. I fixed all the problems you mentioned. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a part of my ongoing work on modernizing the codebase.
On a sidenote, I think that the return of promises may at least be broken in a browser runner - I can't trigger a failure in async code. Unfortunately I lack the ability to debug this.