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

Repair Functional Tests: test for confirm on and off #335

Merged
merged 1 commit into from
Oct 8, 2018

Conversation

jeschu1
Copy link
Member

@jeschu1 jeschu1 commented Oct 4, 2018

Enrich existing tests to include confirm off

Per @kewillford 's request. This change should run quickly and give us added security.


this.Enlistment.TryMountGVFS().ShouldEqual(false, "Repair without confirm should not fix the enlistment");

this.Enlistment.Repair(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Helper method for these three lines?

@@ -23,7 +33,11 @@ public void FixesCorruptHeadSha()

this.Enlistment.TryMountGVFS().ShouldEqual(false, "GVFS shouldn't mount when HEAD is corrupt");

this.Enlistment.Repair();
this.Enlistment.Repair(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually use named parameters for constant values like
this.Enlistment.Repair(confirm: false);

"repair --confirm \"" + this.enlistmentRoot + "\"",
failOnError: true);
string confirmArg = confirm == true ? "--confirm " : string.Empty;
if (confirm)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should be checking this bool otherwise the repair will not be ran when false. We still want to run it just without the --confirm

this.CallGVFS(
"repair --confirm \"" + this.enlistmentRoot + "\"",
failOnError: true);
string confirmArg = confirm == true ? "--confirm " : string.Empty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need the == true here.

public void NoFixesNeeded()
{
this.Enlistment.UnmountGVFS();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another style nit: I think there are an excessive number of blank lines in these tests. It's hard to read code when each line of code is followed by a blank line :-)

@jeschu1 jeschu1 force-pushed the repair_functional_tests branch 2 times, most recently from 45c106f to f00be38 Compare October 4, 2018 15:37
Enrich existing tests to include confirm off
@jeschu1 jeschu1 closed this Oct 5, 2018
@jeschu1 jeschu1 reopened this Oct 5, 2018
@jeschu1 jeschu1 merged commit 1c6a49f into microsoft:master Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants