Enable random order for specs#2466
Merged
maestromac merged 10 commits intoforem:masterfrom Apr 17, 2019
rhymes:rhymes/spec-random-order
Merged
Enable random order for specs#2466maestromac merged 10 commits intoforem:masterfrom rhymes:rhymes/spec-random-order
maestromac merged 10 commits intoforem:masterfrom
rhymes:rhymes/spec-random-order
Conversation
rhymes
commented
Apr 16, 2019
|
|
||
| # Follow has an after_create callback that creates a channel between the two users, | ||
| # so to make sure this test is correct, we delete all channels right after | ||
| ChatChannelMembership.delete_all |
Contributor
Author
There was a problem hiding this comment.
As I specified in the comment, the Follow model has an after_create callback whose side effect is to create a ChatChannel. The job doesn't create a new channel if there's an existing one. I honestly have no idea what was the combination and the sequence of test leading to no channels so that this one would pass but by removing channels explicitly we can ensure the test always passes
Contributor
There was a problem hiding this comment.
The callback contains the call to the job perform_later, which won't execute if the queue_adapter is set to test, so the channel was not actually created in the callback and the test passed.
However, I agree that the previous version of the test is not clear regarding the ChatChannel creation, so explicitly deleting makes sense.
A few tests are running by chance because the correct combination of variables has been set before execution. Some of these "race conditions" have been surfaced by running in order. The queue adapter value is one of them. Also using ActiveJob::TestHelper helpers.
13 tasks
lightalloy
reviewed
Apr 17, 2019
Contributor
lightalloy
left a comment
There was a problem hiding this comment.
Amazing job. Thanks, @rhymes
I thought about the possible problem with the ActiveJob.queue_adapter inconsistency, but haven't worked on it.
lightalloy
approved these changes
Apr 17, 2019
maestromac
reviewed
Apr 17, 2019
7 tasks
coreyja
added a commit
to coreyja/dev.to
that referenced
this pull request
Apr 20, 2019
* master: (83 commits) Update gitdocs (forem#2500) Condense 'ask me anything' to 'ama' (forem#2428) [ci skip] Add user_signed_in? to cache key for styles (forem#2498) Added troubleshooting for byebug without readline issue (forem#2481) Fix some frontend linting issues (forem#2495) [ci skip] Fix <br/> in footer. (forem#2491) Remove extra param and add message for prefill (forem#2487) Make Cards Change Dynamically and add Reader/Follower Charts (forem#2488) Temporarily comment out random (forem#2486) Add nav buttons to pwa desktop (forem#2484) Feature/filtered charts (forem#2482) Add caching for historical data (forem#2476) There are installation sections for other OSes now. (forem#2480) Add inbox guidelines to users (forem#2473) Release Open Inbox (forem#2468) Convert underscores in article slugs properly (forem#2472) Update framework defaults to match those in Rails 5.1 (forem#2309) Enable random order for specs (forem#2466) [ci skip] forem#118 Allow users to embed Medium posts with Liquid Tags (forem#1161) Allow API to return top articles (forem#2469) ... # Conflicts: # Envfile # app/models/user.rb # db/schema.rb
8 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
What type of PR is this? (check all applicable)
Description
Running specs in a random order it's a great way to see if there are dependency problems among tests or weird race conditions.
The great thing about this is that by providing the seed with the failing test in a subsequent run it's possible to reproduce the conditions that lead to the tests failing.
I had to make some modifications to some tests to increase robustness (and get rid of the weird behaviors on sequencing and active job related tests)
Added to documentation?