Only spawn a browser when using the feature macro, not normal tests
#795
Conversation
This file contains 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
This fixes an unintended behavior where having `use Wallaby.Feature` at the top of your module would cause a browser to be spawned for every test in the module, not just `feature` tests. The result is that if you had a large test module, most of which used the normal `test` macro and one of which used the `feature` macro, every test would take a minimum of a third of a second or more. I also took the liberty to check and see in the `setup` block if we already had a `:session`, and if so, avoid spawning a second. I don't *think* anyone was relying on the behavior where a second browser was being spawned if you had `use Wallaby.Feature` at both the top of the module and a `describe` block. Resolves elixir-wallaby#794
mhanberg
requested changes
Dec 29, 2024
lib/wallaby/feature.ex
Outdated
|
|
||
| # If you have `use Wallaby.Feature` at the top of your test module, | ||
| # then also within a `describe` block, we only want to set up one session. | ||
| already_has_session? = is_map_key(context, :session) |
There was a problem hiding this comment.
Let's not deal with this in this pull request and do a second one.
This might be better to emit a warning rather than be silent.
lib/wallaby/feature.ex
Outdated
| end) | ||
| |> unquote(__MODULE__).Utils.build_setup_return() | ||
| # Only set up a session if this is a feature test (using the `feature` macro). | ||
| feature_test? = context[:feature] == true |
There was a problem hiding this comment.
I think here you want to check context[:test_type] == :feature
There was a problem hiding this comment.
Also I would just inline this. It's self explanatory.
|
The tests might fail a bunch but you don't have to worry about it. |
|
Changes made! |
mhanberg
approved these changes
Jan 3, 2025
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 fixes an unintended behavior where having
use Wallaby.Featureat the top of your module would cause a browser to be spawned for every test in the module, not justfeaturetests. The result is that if you had a large test module, most of which used the normaltestmacro and one of which used thefeaturemacro, every test would take a minimum of a third of a second or more.I also took the liberty to check and see in thesetupblock if we already had a:session, and if so, avoid spawning a second. I don't think anyone was relying on the behavior where a second browser was being spawned if you haduse Wallaby.Featureat both the top of the module and adescribeblock.Resolves #794