Stop saving persistence layer across spec tests #1129
Conversation
| @@ -78,6 +80,12 @@ export function setSpecJSONHandler(writer: (json: string) => void): void { | |||
| function getTestRunner(tags, persistenceEnabled): Function { | |||
| if (tags.indexOf(NO_WEB_TAG) >= 0) { | |||
| return it.skip; | |||
| } else if ( | |||
| !persistenceEnabled && | |||
| tags.indexOf(NO_MEMORY_PERSISTENCE) !== -1 | |||
There was a problem hiding this comment.
We should avoid these inversely-phrased tags. They're hard to reason about because they don't state what's required, but rather what's excluded.
This one could be named something like durable-persistence, which would make it more obvious to the reader that this is the property of the persistence layer that's interesting.
There was a problem hiding this comment.
We used to have a tag that was called just 'PERSISTENCE', which might imply that it is durable.
There was a problem hiding this comment.
I agree with the general principle, but I did it this way to match how the rest of the tags were implemented. Do you think it's worth deviating?
There was a problem hiding this comment.
benchmark, exclusive, and multiclient follow the positive form. I think we should continue that.
no-web, no-ios, and no-android are a mistake; I think they should have been phrased the other way too, especially since we tend to add a feature on a new platform, so it's easier to phrase this in terms of which platform is going first.
That leaves no-lru, which I think we could stand to be positive, but I don't know what the positive form would be.
In any case, I think positive phrasing could be a long term goal and this would be a step in that direction.
Also, since you're going to have to edit every runner to implement this, you could just refactor all to positive.
There was a problem hiding this comment.
Switched the tags to be positive.
There was a problem hiding this comment.
Switch the platform tags back to negative.
| // One millisecond later, watch sends an updated resume token but the | ||
| // user doesn't manage to unlisten before restart. | ||
| .watchSnapshots(2000, [], 'resume-token-2000') | ||
| .restart() |
There was a problem hiding this comment.
Could we make restart check that the tag is set? Are there tests that have any business restarting without durable persistence?
There was a problem hiding this comment.
We could make restart check, if we add something like abstract isDurable(): boolean to TestRunner. While implementing this I had an assert for instanceof MemoryPersistence, but decided that was too hacky to check in. I'll add it if you think it's worth it.
There are not currently tests that make sense with non-durable persistence and a restart, but that's also because we couldn't really write them before. One could now write a test for memory persistence behavior across a restart.
There was a problem hiding this comment.
I left out this feature. We can add it if we need it.
Verified we don't lose any code coverage in tests.
Exempt a few spec tests from running with memory persistence. Restart the persistence layer along with everything else in spec tests.