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

test: add activation tests #1269

Merged
merged 4 commits into from Feb 11, 2017

Conversation

Projects
None yet
3 participants
@olevett
Copy link
Member

olevett commented Feb 10, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Some unit tests (and incidentally some whitespace changes) around Activation.

What is the current behavior? (You can also link to an open issue here)
Fewer tests.

What is the new behavior (if this is a feature change)?
More tests.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

Other information:
Got bored and wanted to dig around in Activation a bit more, and ended up with some pretty trivial tests - they should be fast at least.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 10, 2017

Coverage Status

Coverage increased (+0.06%) to 65.938% when pulling d9dcb4b on olevett:activation-tests into 3b97bcc on reactiveui:develop.

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 10, 2017

Hey @olevett. Some of the formatting you've changed is intentionally that way (not my choice - just the way it was agreed earlier in the life of this project). Do you have Rebracer installed? We have a rebracer config file, so as long as you're editing in VS with it installed then it should Just Work.

@olevett

This comment has been minimized.

Copy link
Member Author

olevett commented Feb 10, 2017

I do have Rebracer installed and those were the changes it made...which one is incorrect, I can persuade it back again...

@olevett
Copy link
Member Author

olevett left a comment

@kentcb I've had a double check of my rebracer settings, and think this is matching at least some of the code base. I'm very happy to change whitespace if it doesn't match the expected style, but not sure what I should change it too.

@@ -45,12 +42,14 @@ public DerivedActivatingViewModel()
public class ActivatingView : ReactiveObject, IViewFor<ActivatingViewModel>
{
ActivatingViewModel viewModel;
public ActivatingViewModel ViewModel {
public ActivatingViewModel ViewModel

This comment has been minimized.

@olevett

olevett Feb 10, 2017

Author Member

I think this matches the Rebracer settings (NewLines_Braces_Property) and would be consistent with ReactiveCommand line 572 for example.

@@ -248,8 +247,7 @@ public void CanUnloadAndLoadViewAgain()
locator.InitializeReactiveUI();
locator.Register(() => new ActivatingViewFetcher(), typeof(IActivationForViewFetcher));

using (locator.WithResolver())
{
using (locator.WithResolver()) {

This comment has been minimized.

@olevett

olevett Feb 10, 2017

Author Member

Again, I think this matches Rebracer (NewLines_Braces_ControlFlow is 0) and would match ReactiveList.cs, line 391.

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 10, 2017

Hmm, maybe I'm misremembering but I swear the style used to be:

public string SomeProperty {
    get {
    }
}

But you're right - that doesn't seem to be the case any more. Oh well, I prefer the style dictated by the current Rebracer settings anyway. Please ignore me and carry on soldier!

@olevett

This comment has been minimized.

Copy link
Member Author

olevett commented Feb 11, 2017

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 11, 2017

I think Rebracer is better in one sense because it automated, but worse in another because it relies on VS + people installing it. I don't mind leaving things as is for now, addressing it later if it becomes a problem.

@kentcb kentcb added this to the vNext milestone Feb 11, 2017

@kentcb kentcb changed the title Activation tests test: add activation tests Feb 11, 2017

@kentcb kentcb merged commit 8e9963f into reactiveui:develop Feb 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.06%) to 65.938%
Details
@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 11, 2017

Thanks @olevett!

@olevett olevett deleted the olevett:activation-tests branch Feb 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment