Add Javascript testing base class #904
Conversation
| @@ -0,0 +1,35 @@ | |||
| require 'test_helper' | |||
|
|
|||
| class RegistrarSignInTest < JavascriptIntegrationTest | |||
There was a problem hiding this comment.
I would name it "RegistrarAreaSignInTest", mainly to avoid name clash (I have experienced such clashes before, and it was time consuming to eventually find out that setup/teardown from 2 classes were merged because of the same name of a test class.
test/test_helper.rb
Outdated
| end | ||
|
|
||
| class JavascriptIntegrationTest < ActionDispatch::IntegrationTest |
There was a problem hiding this comment.
I think it's worth following Rails 5 naming convention http://guides.rubyonrails.org/testing.html#system-testing, given we anyway migrate it.
So to name it ApplicationSystemTestCase instead, and place it into separate file under test dir
|
What do you mean run system tests with JS by default? Or rather, what do you mean by system test? I find the Rails nomenclature to be completely arbitrary and just made up because they already had used up the integration test name. In any case, there's a significant performance penalty (3-5 times slower tests, even more) that I think it's a bad idea to do it do it by default, especially since our application uses very little javascript. |
|
https://en.wikipedia.org/wiki/System_testing
Whereas integration tests verify that some parts work together. Seems quite clear for me. |
|
I fully agree regarding speed, but as I mentioned, if it is not run in a real browser with enabled JS by default (using Rack), there is a chance of exactly same false positive, as we had in #893. Any other ideas how to avoid such problem in the future? |
We have to swallow it and when see an existing RackTest case that should use headless Chrome we should convert them as we do it. Otherwise we would open another can of worms. |
In the future, ApplicationSystemTestCase should inherit from ActionDispatch::SystemTestCase and JavaScriptApplicationSystemTestCase could possibly be removed if the `driven_by` method works as it is promised in Rails documentation: http://api.rubyonrails.org/v5.2/classes/ActionDispatch/SystemTestCase.html Consider introducing another class between ActionDispatch::IntegrationTest and other items inheriting from it, as the general Rails practice seems to have `ApplicationIntegrationTest`, as we do have `ApplicationRecord` and `ApplicationController`.
Fixes #900, inspired by false positive from #893 .
A couple gotchas:
:development, :testgem group.JavascriptIntegrationTest.Can be merged straight to master after it is accepted, since it does not have production impact.