Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable laziness in ReactiveCommand and ToProperty in unit test runner #705

Merged
merged 5 commits into from Aug 19, 2014

Conversation

anaisbetts
Copy link
Member

Consider the following VM constructor code:

ToasterList = new ReactiveList<Toaster>();
ToasterList.CollectionCount
    .Select(x => String.Format("{0} toasters!", x))
    .ToProperty(this, x => x.ToasterPrompt, out toasterPrompt);

Then the following test:

viewModel = new MyViewModel();
viewModel.ToasterList.Add(new Toaster());

// Fails! Why????
Assert.Equal("1 toasters!", viewModel.ToasterPrompt);

This fails because in RxUI 6.0, ObservableAsPropertyHelper is now lazy, and doesn't subscribe to its source until the Value is accessed. Since CollectionCount is a Hot Observable, OAPH won't update until the collection count changes one more time.

In the application, this is a good thing - since the person subscribing to the VM is usually a View (via Bind et al), we don't waste subscriptions on things that aren't being displayed to the user.

However, in a test runner, we don't need that efficiency, and we're confusing users who make reasonable assertions then are surprised when they don't work

@anaisbetts
Copy link
Member Author

/cc @reactiveui/owners, any objections?

@tberman
Copy link
Member

tberman commented Aug 19, 2014

I think making the test harness not the same as the app is not a good idea.

@anaisbetts
Copy link
Member Author

I think making the test harness not the same as the app is not a good idea.

I had the same feeling initially, but then I realized - the test harness is already not the same as the app, because there's no View attached to your VMs. This change should make the test runner more similar to the app in general. While there is a risk that you'll now have bugs that only surface in-app, I think that case is pretty rare.

@tberman
Copy link
Member

tberman commented Aug 19, 2014

Maybe add a log.debug or something each time so people understand whats going on?

@tberman
Copy link
Member

tberman commented Aug 19, 2014

(In the test-runner case)

@anaisbetts
Copy link
Member Author

We really need to add a FAQ in the docs about test runners vs. in-app

anaisbetts pushed a commit that referenced this pull request Aug 19, 2014
Disable laziness in ReactiveCommand and ToProperty in unit test runner
@anaisbetts anaisbetts merged commit 2a6dd93 into master Aug 19, 2014
@anaisbetts anaisbetts deleted the eager-in-unit-test-runners branch August 19, 2014 18:57
@lock lock bot locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants