Skip to content

Unit tests for SignToolTask#6746

Merged
chcosta merged 4 commits intodotnet:masterfrom
chcosta:signtooltask-tests
Jan 6, 2021
Merged

Unit tests for SignToolTask#6746
chcosta merged 4 commits intodotnet:masterfrom
chcosta:signtooltask-tests

Conversation

@chcosta
Copy link
Copy Markdown
Member

@chcosta chcosta commented Jan 6, 2021

Fixes #6365

@chcosta chcosta requested a review from missymessa January 6, 2021 19:31
Comment thread src/Microsoft.DotNet.SignTool.Tests/SignToolTests.cs Outdated
[Required]
public string LogDir { get; set; }

internal BatchSignInput ParsedSigningInput { get; private set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume that you made this change in the interest of time to get as many tests done as possible. To not have to expose this value, I would have created a factory pattern to create a new BatchSignUtil, so that I could use a mock to verify the value of the signingInput being sent into it.

WixToolsPath = badPath
};

task.Execute();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to verify that the result of the execute is false? I know we imply that it's false because the log has logged errors in the following assertion, but it would ensure that the results are expected in the event someone decides to refactor the tasks and makes a mistake.

Copy link
Copy Markdown
Member

@missymessa missymessa left a comment

Choose a reason for hiding this comment

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

LGTM shipit

@chcosta chcosta merged commit f791c3d into dotnet:master Jan 6, 2021
@chcosta chcosta deleted the signtooltask-tests branch January 6, 2021 21:43
akoeplinger pushed a commit to akoeplinger/arcade that referenced this pull request Apr 12, 2021
* Unit tests for SignToolTask

* pr feedback: remove unneeded trait

* Add comment

* pr feedback: test task return value
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.

Need tests for SignToolTask in SignTool

2 participants