jejacks0n / teaspoon

Teaspoon: Javascript test runner for Rails. Use Selenium, BrowserStack, or PhantomJS.
1.43k stars 243 forks source link

passing --filter="...." correctly escapes the spaces into "%20" and not "+" #595

Open thebravoman opened 1 year ago

thebravoman commented 1 year ago

PR created against 1.2.2

Running teaspoon as

bundle exec teaspoon -s default --filter="ScenePointers are attached"

When using CGI.escape the string "ScenePointers are attached" becomes "ScenePointesr+are+attached" This is then passed as a "grep=ScenePointers+are+attached" into the url

In the JS that is running the specs in the browser the value of the grep is used to filter. We move through all the specs and encode their full name with encodeURIComponent

The result is

encodeURIComponent("ScenePointers are added")
'ScenePointers%20are%20added'

This means that on the ruby side we were escaping the space " " in the urls as '+' and on the JS side we were escaping them as "%20". One of these had to change and it seems changing it on the ruby side is the right one.

There is no right way to escape. We can use 'addressable' and URI and CGI and ERB. ERB seems to be the most straightforward for this case, it also does not bring a lot of dependencies. Just one - 'cgi'.

In the same time there seems to be no method in URI or CGI or 'addressable' gem that would address this

mathieujobin commented 1 year ago

Did you check why the CI isn't happy ?

thebravoman commented 1 year ago

I did. It was too much and I could not directly see the connection between the change and the fails.

Let me know what you think about the PR. If you think it is in the right direction I could invest some time to understand why the specs fail.

mathieujobin commented 1 year ago

you're right failures, seems quite unrelated

please have a look at https://github.com/jejacks0n/teaspoon/pull/597

and please rebase once merged

adding a test to reproduce the problem you are fixing would be great

thebravoman commented 1 year ago

Thanks for the feedback. Now as this makes sense I will think of a spec, rebase and clean up the teaspoon.gemspec

mathieujobin commented 1 year ago

please update your branch with latest master