Test improvements #174
Merged
Test improvements #174
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
We were testing from recovery from a full flush, but it is also possible for a backlog to expire while message ID counters persist.
This test was introduced in discourse@9c16e7e . Its purpose does not appear to be to assert that the forked process has a subscription (since no code is present to ensure that delivery ocurrs to both processes), nor does the order of publication or delivery appear to be important in this spec. All we're really interested in asserting here is that publications before a fork, from the forked process, and from the original process after the fork ocurrs, all get published and delivered. This change expresses that case much more simply. Prior to this change, this test would fail on Redis approximately 20% of the time. With this change, it did not fail at all in 30 executions.
These specs were all being applied only to specific backends. Several of them are (functionally) the same, and do not need to be maintained separately. Those tests which do not apply to all backends are now clearly indicated by calling `test_only [some backend(s)]`, forming a todo list for building feature parity.
|
@SamSaffron If you are amenable to these changes, I would prefer that they be merged first and #175 subsequently require conflict resolution than the other way around. Thanks for your time! |
The previous tests simply asserted that the backend-specific mechanism for expiring backlogs on Redis was set up, but not that it actually worked. The test could not, then, be exercised on other backends, whose implementations of backlog expiry do not have the same behaviour as those of the Redis backend. These tests now assert the behaviour of the Redis backend, but do not invoke any private APIs specific to Redis, and can thus be applied to other backend implementations.
benlangfeld
force-pushed
the
test-improvements
branch
from
ddc0e21 to
3c656ac
Compare
This allows the tests to assert that the preferred behaviour is for expiry to be automatic at the correct time, and that this ocurrs on compatible backends. Conversely, the tests assert that backends which require publication-triggered expiry do not auto-expire; if they gain such functionality in the future, the tests would have to be updated to reflect this, allowing us to work towards eliminating this divergence in behaviour from the tests.
SamSaffron
reviewed
Nov 7, 2018
|
|
||
| @bus.pub_redis.flushdb | ||
| @bus.reset! |
There was a problem hiding this comment.
I much prefer reset methods, way cleaner
SamSaffron
reviewed
Nov 7, 2018
|
|
||
| @bus.global_backlog.length.must_equal 1 | ||
|
|
||
| sleep 1.25 # Should now be at time =~ 1.25s. Our backlog should have expired by now. |
There was a problem hiding this comment.
sleeps in tests make me uneasy, is there any other option here. even if we give a special "rush" method o back ends or something.
There was a problem hiding this comment.
I'm not aware of a way to wind Redis' clock forward. The point of this is to actually exercise Redis' expiry rather than fake it in message_bus.
|
Overall I am fine with the changes it improves the test suite a lot 👍 |
|
@SamSaffron If you're happy, I'm ready for this to be merged. I have another PR coming up to improve feature-parity. |
|
Sure lets go for it! Thanks Ben |
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.
Some of this was extracted from the work on Redis Streams support. I think it's worthwhile even before that happens.
Other parts generally improve the structure of the test suite (duplication of tests and conditional test definition rather than execution), it's communication of intent (a simpler forking spec) and its stability (avoiding regular failure in the forking spec on Redis).