Add integration tests for starting phantom and chromedriver #512
Conversation
aaronrenner
force-pushed
the
ar-script-driven-integration-tests
branch
from
4d3147a to
34038a5
Compare
| ensure_setting_is_reset(:wallaby, :chromedriver) | ||
| Application.put_env(:wallaby, :chromedriver, path: test_script_path) | ||
|
|
||
| assert :ok = Application.start(:wallaby) |
There was a problem hiding this comment.
Are all these lines necessary since the stop_wallaby function flunks if the application won't start?
There was a problem hiding this comment.
The flunk in stop_wallaby is in an on_exit callback, so the flunk will occur only if it can't restart wallaby after the test is done.
Since wallaby reads its config and starts worker pools on Application startup, in order to change settings for the driver we need to:
- Stop wallaby
- Adjust the environment for the new script path/settings
- Start wallaby back up
In the future, I think it would be really nice to allow more overrides in Wallaby.start_session (like which driver to use, whether or not to start a chromedriver instance or use a remote instance, etc), instead of relying so heavily on the application config that we're having to start and stop the full application during tests and can only use one driver (phantom, selenium, chromedriver) at a time. That's definitely outside the scope of this PR and should be discussed more, but I figured I just should share what I've been thinking for the longer term.
There was a problem hiding this comment.
Is Wallaby not already started at the beginning of the test since it's restarted in the on_exit callback? Or does calling Application.start/1 restart the application? (I was thinking it nooped if it was already started)
There was a problem hiding this comment.
I stopped wallaby before the test is run on line 14 in the setup block:
setup [:stop_wallaby, :create_test_workspace]I figured that way I can start the tests by configuring wallaby before I start it back up (and so I don't forget to stop it and have my config changes not matter). Now that I look at it, I do see that setup step could be easy to miss, though. Is there another way I should write this that would be more clear?
There was a problem hiding this comment.
🤦♂
I realized that the on_exit through me off, in reality, the steps are
- stop wallaby
- reset env stuff
- start wallaby
- test
- stop wallaby
- start wallaby
I might split it up into two functions. One stops wallaby stop_wallaby and the other defines the on_exit and is named restart_on_exit! or something like that.
There was a problem hiding this comment.
I split it into 2 functions in the setup block: stop_wallaby and restart_wallaby_on_exit!. Do you prefer Application.stop(:wallaby) as the first line in each test instead of having a function that's called during setup?
aaronrenner
force-pushed
the
ar-script-driven-integration-tests
branch
from
960b7b2 to
424d89d
Compare
Instead of just deleting config, it would be better to reset it back to the previous state.
aaronrenner
force-pushed
the
ar-script-driven-integration-tests
branch
from
24d0098 to
648c359
Compare
| on_exit(fn -> | ||
| Application.delete_env(:wallaby, :chromedriver) | ||
| end) | ||
| ensure_setting_is_reset(:wallaby, :chromedriver) |
There was a problem hiding this comment.
This adds integration tests for around starting wallaby and the underlying phantomjs and chromedriver processes. These tests establish a pattern of how to test a webdriver server's startup and will make it easier to work on the following issues:
I chose to test this via integration tests because I'm thinking this style of test will give us the most flexibility to address #423 while having test coverage to ensure wallaby as a whole continues to function as users expect.